-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[FIX] Highlight active button in sidebar #13111
Conversation
Is there a way to replace the padding around the list with a padding inside the items so that the active item looks like when it is hovered? |
Yep, I have to somehow refer to the parent element of the |
@supra08 Maybe |
07e72b1
to
cb5e010
Compare
@tassoevan, I've modified it. Please have a look. |
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.
I've requested only one change, but I'd like to add a suggestion here: when the account page is open, the Preferences item is selected by default but the sidebar item is not highlighted.
'click [data-id], click .sidebar-item__link'(e) { | ||
const element = e.currentTarget; | ||
const sidebarElements = document.getElementsByClassName('sidebar-item'); | ||
for (let i = sidebarElements.length - 5; i < sidebarElements.length; i++) { |
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 shouldn't rely on magic constants in expressions like sidebarElements.length - 5
. What about using a more scoped query selector get the items?
const sidebarElements = document.querySelectorAll('.sidebar .flex-nav .sidebar-item');
for (const sidebarElement of Array.from(sidebarElements)) {
...
}
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.
Alright, I'll make the changes and push asap.
@tassoevan, changes done ^ |
@tassoevan can you please help me with this? After merging the latest develop, a test is failing. Prior to this the code was working smoothly and the PR is ready. |
c35ef5f
to
b668f2b
Compare
Any news about this pull? |
@tassoevan I dont think there is a need to loop around the flex tabs to do the highlight. The highlight feature is already defined in "sidebarItem" Template. |
I'm closing this PR in favor of #17047. Many things have changed on how we manage the routes deprecating the approach used here. 😞 |
Closes #8500
This PR adds highlights to the sidebar items in the My Accounts page to make it easy to identify which section is open.
@ggazzo @tassoevan please review.