NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
11 Feb 2019

Pull Request for Issue # .

Summary of Changes

  • Eslint
  • Proper event listeners

Testing Instructions

Create a new article and check the image in tinyMCE and also the intro/fulltext image field

Expected result

Actual result

Documentation Changes Required

Nope

avatar dgrammatiko dgrammatiko - open - 11 Feb 2019
avatar dgrammatiko dgrammatiko - change - 11 Feb 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 11 Feb 2019
Category JavaScript Repository
avatar zwiastunsw
zwiastunsw - comment - 13 Feb 2019

Beyond the subject:
Why can't we use sr-only class? Is the class name reserved for Bootstrap? This is just a CSS class name.
If we can't use this class, let someone decide which class to use. I don't see any sense in changing the sr-only name to only-sr or any other name just to make it different from the Bootstrap if you want to do exactly the same thing. But if you need to change, you need to change, not wait for Godot.

avatar dgrammatiko
dgrammatiko - comment - 13 Feb 2019

@zwiastunsw @brianteeman you have to think a little bit here why Custom Elements should not have any dependency

So here a simple explanation:
In J3 you could have any template that's not based on Bootstrap. All works fine until your template needs to render any of the Joomla Form Fields. Then the problems arise and you're facing a really big task: rewrite (override) all the interactive fields (eg fields that have some Javascript)

Now for Joomla 4 we don't wanna force such stupid limitation that will lock designers to use Bootstrap whether they want to or not. So what was the solution we all agree on? Use custom elements which by definition are (or supposed to be) simple snippets of code without any dependency.

So on this sr-only issue (not really): this class is owned by Bootstrap, but we can use it (meaning we also copy the contents in the elements css, remember the aim is that these fields should work correctly whatever the rest of the css in the page) or do something more sensible: https://github.com/joomla/joomla-cms/pull/23215/files#diff-b0459aacb370f1ded9933a88103371b4R70

PS: if you're going to say but you're repeating this chunk of code, well its better to have couple lines of css per component and have this component framework free than limiting Joomla to Bootstrap (or even worse loading Bootstrap on top of any other framework)

I hope this explanation is enough...

avatar dgrammatiko dgrammatiko - change - 13 Feb 2019
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 13 Feb 2019
Category JavaScript Repository JavaScript Repository Layout
avatar dgrammatiko
dgrammatiko - comment - 13 Feb 2019

@zwiastunsw @brianteeman so how about now:
screenshot 2019-02-13 at 12 54 51

avatar zwiastunsw
zwiastunsw - comment - 13 Feb 2019
  1. My comment did not refer to the case of this PR, but to the issue of using sr-only class at all.

  2. Icons added via CSS are usually not announced by screen readers. But assistive technologies behave differently. We don't have a chance to check every behaviour. Therefore, it is recommended that you always add the aria-hidden attribute if you want to be sure that the screen reader will not announce an icon. See this post: https://www.filamentgroup.com/lab/bulletproof_icon_fonts.html

  3. The role="presentation" refers only to the tag to which it has been written, but not to child tags.
    Children's items retain their properties. Of course, this does not apply to the whole structure, e.g. list, table or tree and their offspring. If a tag table has the attribute role=presentation", then, of course, the whole structure, including the elements tr, td, tbody, thead, tfoot are treated as presentation elements. But if you add role="presentation" to the div element, you don't cause everything that is inside to be treated by screen readers as a presentation only.

Make simple text. Create an HTML document, place an image inserted with the img tag inside the div tag. The image will be announced by the screen reader. If you skip alt, of course, it will be ignored, but it will simply be an accesibility error.

role="presentation" for the tag div does not make sense at all. Because the div tag has no implicite role assigned to it. This is generic container.

I will test this PR and let you know if it is OK. On the basis of the code I cannot say much.

avatar dgrammatiko
dgrammatiko - comment - 13 Feb 2019

FWIW I tried to fix the many problems of relying on an icon font, which obviously is far away from the todays best practices, last year but there was a decision to keep the FA font for the back end.

For reference: joomla/40-backend-template#441

I still believe this should be the way for the icons in joomla 4, not an icon font...

avatar zwiastunsw
zwiastunsw - comment - 13 Feb 2019

It doesn't matter if you use the font icon or svg. In both cases, you have to make sure that the meaning is conveyed if it is important and that you hide it from the screen readers.

avatar dgrammatiko
dgrammatiko - comment - 13 Feb 2019

@zwiastunsw the topic goes way out of the scope of this PR but let me rewrite what is happening here:

  • the icon was removed and replaced by a background image, thus sr-only is inappropriate here:

screenshot 2019-02-13 at 17 07 29

src: https://webaim.org/techniques/alttext/

  • The role="presentation" is useless as well, the only thing needed here is an alt="" for the selected image
avatar zwiastunsw
zwiastunsw - comment - 13 Feb 2019

Yes! I agree!

avatar joomla-cms-bot joomla-cms-bot - change - 13 Feb 2019
Category JavaScript Repository Layout JavaScript Repository
avatar joomla-cms-bot joomla-cms-bot - change - 27 Feb 2019
Category JavaScript Repository JavaScript Repository NPM Change
avatar wilsonge
wilsonge - comment - 27 Feb 2019

@SniperSister @zero-24 RIPS please

avatar SniperSister
SniperSister - comment - 28 Feb 2019

Fixed

avatar wilsonge
wilsonge - comment - 28 Feb 2019

Thanks!

avatar wilsonge wilsonge - change - 28 Feb 2019
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-02-28 10:23:57
Closed_By wilsonge
Labels Added: NPM Resource Changed
avatar wilsonge wilsonge - close - 28 Feb 2019
avatar wilsonge wilsonge - merge - 28 Feb 2019

Add a Comment

Login with GitHub to post a comment