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

Incorrect current section highlighting on partial match #1237

Closed
sapegin opened this issue Dec 24, 2018 · 3 comments
Closed

Incorrect current section highlighting on partial match #1237

sapegin opened this issue Dec 24, 2018 · 3 comments

Comments

@sapegin
Copy link
Member

sapegin commented Dec 24, 2018

Current behavior

Several sections are highlighted when the current section contains part of a name of a section in the sidebar.

To reproduce

image 2018-12-24 at 7 57 53 pm

Expected behavior

Only one section is highlighted.

@guyius
Copy link
Contributor

guyius commented Dec 28, 2018

Hey @sapegin !
I started looking into this issue and found the root cause, Do you mind if I work on this?
It's going to be a first-time open source PR for me.

@sapegin
Copy link
Member Author

sapegin commented Dec 28, 2018

That would be awesome! Let me know if you need any help :-)

@guyius
Copy link
Contributor

guyius commented Dec 28, 2018

Okay so the issue lies in the fact that the ComponentsListRenderer sets the isSelectedItem const according to the hasHash function, which checks for indexof, Thus Nav returns true for routes like NavLink. I reproduced it by changing CounterButton to ButtonCounter for example and colliding with Button.
IMO we can simplify this and check in ComponentsListRenderer for equality by comparing the 2 strings Instead of doing this: const isItemSelected = hasInHash(windowHash, href);
We can simply do this: const isItemSelected = windowHash === href;
Questions: 1. I prefer this simple logic to live in the ComponentsListRenderer seems to make sense for me, WDYT?
2. All tests pass with this change, and I saw that all the tests for ComponentsListRenderer are snapshots, Which also missed this edge case in the first place. Should I add specific tests for this edge case or leave this as is with the snapshot tests only?

guyius added a commit to guyius/react-styleguidist that referenced this issue Dec 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants