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

[FIX] Highlight active button in sidebar #13111

Closed
wants to merge 12 commits into from

Conversation

supra08
Copy link
Contributor

@supra08 supra08 commented Jan 10, 2019

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.

bcyyd-qw887

@ggazzo @tassoevan please review.

@tassoevan
Copy link
Contributor

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?

@supra08
Copy link
Contributor Author

supra08 commented Jan 11, 2019

Yep, I have to somehow refer to the parent element of the <a>, i.e. the <li>, on which the css is applied.

@tassoevan
Copy link
Contributor

@supra08 Maybe const element = e.currentTarget. parentElement; will do the trick.

@supra08 supra08 force-pushed the active-link-highlight branch from 07e72b1 to cb5e010 Compare January 11, 2019 13:54
@supra08
Copy link
Contributor Author

supra08 commented Jan 11, 2019

@tassoevan, I've modified it. Please have a look.
rocket_pr_1

Copy link
Contributor

@tassoevan tassoevan left a 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++) {
Copy link
Contributor

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)) {
  ...
}

Copy link
Contributor Author

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.

@supra08
Copy link
Contributor Author

supra08 commented Jan 12, 2019

@tassoevan, changes done ^
rocket_2

@supra08
Copy link
Contributor Author

supra08 commented Jan 21, 2019

@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.
Edit: I forced pushed the last commit before the merge and it's passing the tests. So can you please help me with that.

@supra08 supra08 force-pushed the active-link-highlight branch from c35ef5f to b668f2b Compare January 21, 2019 07:44
@kevinfaveri
Copy link

Any news about this pull?

@tassoevan tassoevan added this to the 1.4.0 milestone Aug 1, 2019
@tassoevan tassoevan modified the milestones: 2.0.0, 2.1.0 Aug 27, 2019
@engelgabriel engelgabriel removed this from the 2.1.0 milestone Oct 13, 2019
@ashwaniYDV
Copy link
Contributor

@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.
Please review this PR #16300

@engelgabriel engelgabriel modified the milestones: 2.3.0, 3.1.0 Mar 17, 2020
@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@tassoevan
Copy link
Contributor

I'm closing this PR in favor of #17047. Many things have changed on how we manage the routes deprecating the approach used here. 😞

@tassoevan tassoevan closed this Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Active link not highlighted in sidebar
7 participants