-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Fixed issue .ui-state-focus missing from main navigation in 2.4.5 and 2.4.6 #37571
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
Conversation
Hi @rostilos. Thank you for your contribution! Add the comment under your pull request to deploy test or vanilla Magento instance:
❗ Automated tests can be triggered manually with an appropriate comment:
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
47fb356
to
65fc2ca
Compare
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
@rostilos I don't think just styling the |
@schizek And yes, both the parent component and the child component are given the same class. This may not be correct, but is it worth rewriting the core functionality with a plugin to do this? If this is an acceptable option, I can do it this way as it was in an older version, just clarifying. |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -99,7 +99,7 @@ | |||
|
|||
&.active { | |||
.all-category { | |||
.ui-state-focus { | |||
.ui-state-active { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change the class name in backward-compatible way like the below:
.ui-state-active, .ui-state-focus
We have verify PR changes and follow steps for verification mentioned here. But with PR when focus is on a menu/submenu item a class should be added of "ui-state-focus" ( Also the menu item with focus must be visually marked ) does not work. Please have a look at below video and let us know if we have missed anything here screen-capture.1.mp4 |
@engcom-Echo Yes, I have looked at it, and will try to fix it as soon as possible, can't spare the time right now, but will give an update soon. Thanks |
@rostilos did you get a chance to have a look on #37571 (comment)? |
As PR need some work to be done based on #37571 (comment) Moving this PR in draft state. |
Description (*)
The bug was reproduced because the updated version of jquery-ui menu widget now adds the "ui-state-active" class instead of "ui-state-focus" to the element with focus. When the jquery-ui library was updated (somewhere between version 2.4.3 and 2.4.5) these edits were added but the styles were not changed.
In this PR, the selectors for the "ui-state-focus" class have been replaced by "ui-state-active" because the "ui-state-focus" class is no longer added to the menu item with focus in the menu functionality of the jquery-ui library widget
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)