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.2] accessibility plugin update and fixes #38009

Merged
merged 8 commits into from
Jun 14, 2022

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Jun 8, 2022

Summary of Changes

#36948 changed the icons in the accessibility from the google material font (because :privacy :facepalm) to use emojis.
Fixes #37050
Fixes #36311

At the time this also resolved a bug in the upstream code that prevented the material font working on osx

Upstream has now resolved the osx problem.

This PR updates to the latest release of the script and introduces the ability to use either emoji or material icons. The default is emoji so there is no visible change etc on upgrade and no need for an update sql.

It;s now a user choice if they want to use the better material font icons

Testing Instructions

Either use a prebuilt package or apply the pull request and then npm i

Expected result AFTER applying this Pull Request

New Option in the plugin
image

With Emojis

emoji
.

With Google Material Font

material

joomla#36948 changed the icons in the accessibility from the google material font (because :privacy :facepalm) to use emojis.

At the time this also resolved a bug in the upstream code that prevented the material font working on osx

Upstream has now resolved the osx problem.

This PR updates to the latest release of the script and introduces the ability to use either emoji or material icons. The default is emoji so there is no visible change etc on upgrade and no need for an update sql
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.1-dev labels Jun 8, 2022
@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on 915bf8a


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

@richard67
Copy link
Member

I've restored the previous human test in the issue tracker since the commit which has invalidated the test count was just a code style change.

@Quy
Copy link
Contributor

Quy commented Jun 11, 2022

I have tested this item ✅ successfully on b690128


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

@Quy
Copy link
Contributor

Quy commented Jun 11, 2022

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 11, 2022
@laoneo
Copy link
Member

laoneo commented Jun 13, 2022

Any reason you made this for 4.1? As this introduces a new feature it should go into 4.2.

@brianteeman
Copy link
Contributor Author

Because it fixes a bug that should never have been merged

@laoneo
Copy link
Member

laoneo commented Jun 13, 2022

But then I would split the pr into a bug fix one and a new feature, when you want to have the bug fix into 4.1. As soon as a pr has a new feature, it should come into the next minor, even when it also fixes a bug.

@brianteeman
Copy link
Contributor Author

thats just a waste of time.

@laoneo
Copy link
Member

laoneo commented Jun 13, 2022

Putting it back to pending


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

@laoneo laoneo added Updates Requested Indicates that this pull request needs an update from the author and should not be tested. and removed RTC This Pull Request is Ready To Commit labels Jun 13, 2022
@brianteeman
Copy link
Contributor Author

should still be rtc and set to RLDQ

@laoneo
Copy link
Member

laoneo commented Jun 13, 2022

I'm putting it back to RTC when rebased.

@brianteeman
Copy link
Contributor Author

4.1 is the correct branch

@laoneo laoneo changed the base branch from 4.1-dev to 4.2-dev June 14, 2022 05:57
@laoneo laoneo changed the title [4.1] accessibility plugin update and fixes [4.2] accessibility plugin update and fixes Jun 14, 2022
@laoneo laoneo removed the PR-4.1-dev label Jun 14, 2022
@laoneo
Copy link
Member

laoneo commented Jun 14, 2022

Can you rebuild the lock file, so we can merge it. Thanks.

@laoneo
Copy link
Member

laoneo commented Jun 14, 2022

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 14, 2022
@laoneo laoneo removed the PR-4.1-dev label Jun 14, 2022
@richard67 richard67 removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jun 14, 2022
@richard67
Copy link
Member

@laoneo Now as it has been rebased, I have removed the "Updates Requested" label which you had added before the rebase.

@laoneo
Copy link
Member

laoneo commented Jun 14, 2022

For safety I would recreate the lock file, that's why I didn't remove it. See my comment here #38009 (comment).

@richard67 richard67 added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jun 14, 2022
@richard67
Copy link
Member

@laoneo Ok, added the label back.

@laoneo laoneo removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jun 14, 2022
@laoneo laoneo merged commit f58bee8 into joomla:4.2-dev Jun 14, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 14, 2022
@brianteeman brianteeman deleted the emojia11y branch June 14, 2022 08:06
@laoneo
Copy link
Member

laoneo commented Jun 14, 2022

Thanks for your cooperation!!

@laoneo laoneo added this to the Joomla 4.2.0 milestone Jun 14, 2022
@brianteeman
Copy link
Contributor Author

well i didnt have a choice even though it is completely wrong to merge into 4.2

@sash85
Copy link

sash85 commented Jan 23, 2023

Hello, i'm not sure if this is the right place for this suggestion. So i'm happy to get Feedback if not.

In my opinion it would be a better way to load the material icons local and use these, instead of the emoji version.
I tried a simple hack and uploaded a material-icons.css and the icon font to my server and replace the the url with the fonts.goolge… .

Everthing works and i can use the material icons (which are much easier to see in my eyes). Do you think it is possible to add another dropdown option (e.g. Material Icons local) with a hint where to download the Font and a CSS Sample to include and where to place it on the server (e.g. in "my-template/css")? Or to upload the font directly in plugin options?

If not, is it possible to create an overwrite for media/vendor/accessibility/js/accessibility.min.js ?
I didn't find any option to do this.

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 NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
7 participants