Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Changing title to tooltip for template preview #33292

Merged
merged 7 commits into from
May 29, 2021

Conversation

infograf768
Copy link
Member

Summary of Changes

As title says.

Testing Instructions

Load Template styles manager and switch Options => Preview Module Positions on and off.
Same for Templates Manager

Hover the Preview icon

Actual result BEFORE applying this Pull Request

Screen Shot 2021-04-25 at 08 49 11

Screen Shot 2021-04-25 at 08 50 34

Screen Shot 2021-04-25 at 08 51 43

Expected result AFTER applying this Pull Request

Screen Shot 2021-04-25 at 08 53 26

Screen Shot 2021-04-25 at 09 06 41

Screen Shot 2021-04-25 at 08 53 58

Documentation Changes Required

@ghost
Copy link

ghost commented Apr 25, 2021

I have tested this item ✅ successfully on 352b4c3


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33292.

@brianteeman
Copy link
Contributor

This is not correct. Please test with a screen reader.

@infograf768
Copy link
Member Author

  1. I have no screenreader except Mac VoiceOver.
    Not sure what would be the correct preferences but using default I tested it on Firefox. It does not read the title either.
  2. We have other occurrences in admin with similar code and VoiceOver does not read the tips either.

If it is no correct, then propose something that works.

@infograf768
Copy link
Member Author

infograf768 commented Apr 26, 2021

Explaining more.
When we have a link I can add an aria-labelledby and it looks like working

<a href="<?php echo Route::link($client, 'index.php?tp=1&templateStyle=' . (int) $item->id); ?>" target="_blank" class="jgrid" aria-labelledby="preview-<?php echo (int) $item->id; ?>">
											<span class="visually-hidden"><?php echo Text::_('COM_TEMPLATES_TEMPLATE_PREVIEW'); ?></span>
										</a>
										<div role="tooltip" id="preview-<?php echo (int) $item->id; ?>"><?php echo Text::sprintf('JBROWSERTARGET_NEW_TITLE', $item->title); ?></div>

But I have no idea how to do when we have no link. I.e. when no preview.

<?php else : ?>
										<span class="icon-eye-slash" aria-hidden="true"></span>
										<div role="tooltip" id="nopreview-<?php echo (int) $item->id; ?>"><?php echo Text::_('COM_TEMPLATES_TEMPLATE_NO_PREVIEW'); ?></div>
										<span class="visually-hidden"><?php echo Text::_('COM_TEMPLATES_TEMPLATE_NO_PREVIEW'); ?></span>
									<?php endif; ?>

@brianteeman
Copy link
Contributor

What is the purpose of <span class="visually-hidden"><?php echo Text::_('COM_TEMPLATES_TEMPLATE_PREVIEW'); ?></span>

@infograf768
Copy link
Member Author

infograf768 commented Apr 26, 2021

What is the purpose of <span class="visually-hidden"><?php echo Text::_('COM_TEMPLATES_TEMPLATE_PREVIEW'); ?></span>

I did not touch that, It was there before as you can see in the diff.

@infograf768
Copy link
Member Author

So normally, the PR is now OK by adding aria-labelledby.

anything else can be modified if necessary in another PR.

Now preparing another PR to use aria-labelledby for associations badges.

@infograf768 infograf768 changed the title [4.0] Changing title to tooltip fpr template preview [4.0] Changing title to tooltip for template preview Apr 26, 2021
@brianteeman
Copy link
Contributor

I did not touch that, It was there before as you can see in the diff.

Maybe not but your changes have an impact on the existing code

@infograf768
Copy link
Member Author

no idea what you mean. Want me to also close that one on a useless comment?

@brianteeman
Copy link
Contributor

OK let me try again with a picture after applying the PR

image

So the point is that you should have touched that line as it is now redundant

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Apr 28, 2021
@RickR2H
Copy link
Member

RickR2H commented May 3, 2021

I have tested this item ✅ successfully on edf8bbf


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33292.

1 similar comment
@ghost
Copy link

ghost commented May 4, 2021

I have tested this item ✅ successfully on edf8bbf


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33292.

@infograf768
Copy link
Member Author

@brianteeman
Do you mean that
class="visually-hidden"> should be deleted?

@brianteeman
Copy link
Contributor

brianteeman commented May 11, 2021

No I mean that the entire line should be deleted as there is no need to have it twice

image

(ignore the highlight in the image)

@brianteeman
Copy link
Contributor

(PS hope you are well - not seen you online for longer than usual)

@richard67
Copy link
Member

@sandramay0905 @RickR2H Could you test this PR again? It has received changes. Thanks in advance.

@infograf768
Copy link
Member Author

Deleted redundant span.

Afterthought:
Using JBROWSERTARGET_NEW_TITLE="Open %s in new window" may be not specific enough.

What do you think about using a new string:
COM_TEMPLATES_TEMPLATE_NEW_PREVIEW="Preview %s in new window"

@brianteeman
Copy link
Contributor

Open or Preview - both are fine - whichever you prefer is fine by me

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label May 12, 2021
@infograf768
Copy link
Member Author

Used new string

Screen Shot 2021-05-12 at 10 34 06

Screen Shot 2021-05-12 at 10 33 44

can be tested now

@richard67 richard67 removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label May 12, 2021
@ghost
Copy link

ghost commented May 12, 2021

I have tested this item ✅ successfully on 5442371


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33292.

<?php else : ?>
<span class="icon-eye-slash" aria-hidden="true" title="<?php echo Text::_('COM_TEMPLATES_TEMPLATE_NO_PREVIEW'); ?>"></span>
<span class="visually-hidden"><?php echo Text::_('COM_TEMPLATES_TEMPLATE_NO_PREVIEW'); ?></span>
<a href="#" aria-labelledby="nopreview-<?php echo (int) $item->id; ?>">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is missing </a>. Plus it gives the impression that the button is clickable when it is not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed. We do not need the <a at all.

@infograf768
Copy link
Member Author

Thanks @Quy
I added the labelledby directly in the span. I guess that would be OK as it also takes care of the cursor.

tip

@infograf768
Copy link
Member Author

@sandramay0905 @RickR2H Could you test this PR again? Modified some aspects. test specially when no preview in template styles.

@ghost
Copy link

ghost commented May 13, 2021

I have tested this item ✅ successfully on b57d245


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33292.

@ghost
Copy link

ghost commented May 14, 2021

@RickR2H can you please retest?

@Quy
Copy link
Contributor

Quy commented May 27, 2021

I have tested this item ✅ successfully on 490cfb0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33292.

@Quy
Copy link
Contributor

Quy commented May 27, 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33292.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 27, 2021
@richard67 richard67 added this to the Joomla 4.0 milestone May 27, 2021
@chmst chmst merged commit 2f39f86 into joomla:4.0-dev May 29, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 29, 2021
@chmst
Copy link
Contributor

chmst commented May 29, 2021

Thanks!

bembelimen pushed a commit that referenced this pull request Nov 23, 2021
* [4.0] Notification icons (#34226)

* remove method allow parent to run (#34118)

Co-authored-by: Richard Fath <[email protected]>

* [4.0] Changing title to tooltip for template preview (#33292)

* [4.0] Changing title to tooltip fpr template preview

* Update location path in description (#34238)

* Fix patterns field check when field empty and not required (#34124)

* [4.0] Finish transition from CSS classes "label-" to "alert-" for the pre-update check (#34227)

* [4.0] mod_popular with disabled hits (#34257)

* [4.0] mod_popular with disabled hits

When hits are disabled for articles it makes no sense to display the Most Read Articles module in the site or admin as the contents will never be updated.

This PR changes the output of the module so that instead of a list of non-updating articles a message is displayed.

* [4.0] Use MVCFactory to create model (#34092)

* Use MVCFactory to create models

* CS

* Fix row selecting, when module is disabled (#34273)

* Media web service implementation.

* Media web service implementation.

* Fix hard coded adapter name default.

* Fix missing url and tempUrl attributes in single item response.

* Remove useless comments.

* Replace 'PATH' with 'STRING' for cleaning of input parameters.

* Adds required parameter checks and removes global exception handling.

* Adds media web service specific language strings.

* Adds exception handling.

* Adds missing exception handler and fixes wrong path return value after move/rename.

* Use exception message as error title.

* Adds proper FileNotFound exception handling for GET requests.

* Fixes handling of status code.

* Fixes some types, formatting and similar stuff.

* Fixes yet another sloppy typo.

* restore file

* Fix some docs

* Adapter endpoint

* Add plugin to install file

* Add tests

* Update api/components/com_media/src/Helper/MediaHelper.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Helper/MediaHelper.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Model/MediaModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Model/MediumModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/View/Media/JsonapiView.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update libraries/src/Error/JsonApi/SaveExceptionHandler.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Controller/MediaController.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Controller/MediaController.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Model/AdaptersModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update api/components/com_media/src/Model/MediaModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* docs

* Update api/components/com_media/src/View/Media/JsonapiView.php

Co-authored-by: Phil E. Taylor <[email protected]>

* int

* cs

* cs

* more cleanup

* tabs

* restore

* Test all endpoints

* Update api/components/com_media/src/Model/MediaModel.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Hardening

* Tests fix for drone

* Update Api.php

* Create directory in test with correct permissions

* Add more tests and remove static helper

* Update tests/Codeception/api/com_media/MediaCest.php

Co-authored-by: Phil E. Taylor <[email protected]>

* Update tests/Codeception/_support/Helper/Api.php

Co-authored-by: Phil E. Taylor <[email protected]>

* cs

Co-authored-by: Brian Teeman <[email protected]>
Co-authored-by: Phil E. Taylor <[email protected]>
Co-authored-by: Richard Fath <[email protected]>
Co-authored-by: infograf768 <[email protected]>
Co-authored-by: Quy <[email protected]>
Co-authored-by: Ruud <[email protected]>
Co-authored-by: Tuan Pham Ngoc <[email protected]>
Co-authored-by: Fedir Zinchuk <[email protected]>
Co-authored-by: Pieter-Jan de Vries <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants