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

Reader: fix sidebar list highlight #1535

Merged
merged 3 commits into from
Dec 22, 2015
Merged

Conversation

bluefuton
Copy link
Contributor

Ensure that only the current list is highlighted in the Reader sidebar.

Previously, it was possible for multiple lists to be highlighted at the same time if they shared the same beginning to their URL (fixes #1523).

screen shot 2015-12-14 at 14 05 46

screen shot 2015-12-14 at 14 05 41

### To test

Create two Reader lists with a common start to their name (e.g. giraffe1 and giraffe2).

Choose the list in the sidebar, and ensure that only the chosen list is highlighted.

Then choose the Manage button for your chosen list, again ensuring that only the chosen list is highlighted.

@bluefuton bluefuton added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. labels Dec 14, 2015
@bluefuton bluefuton self-assigned this Dec 14, 2015
@bluefuton bluefuton added this to the Reader: List Management milestone Dec 14, 2015
@blowery
Copy link
Contributor

blowery commented Dec 14, 2015

Still seems a bit buggy. Here's what I'm seeing:

https://cloudup.com/czFzsIbJiF2

@blowery blowery force-pushed the fix/reader-sidebar-highlight branch from e0ba6a2 to 48078b9 Compare December 14, 2015 14:51
@blowery
Copy link
Contributor

blowery commented Dec 14, 2015

📌 Rebased this after the React 0.14 merge and force pushed

@bluefuton
Copy link
Contributor Author

https://cloudup.com/czFzsIbJiF2

Ah. The endsWith comparison isn't going to fix cases where the slugs have and common start and end... (e.g. blahblah and blahblahblah)

I think we just need to strip the last URL segment and compare that. I'm on it.

@bluefuton bluefuton added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 14, 2015
@bluefuton bluefuton added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 15, 2015
@bluefuton
Copy link
Contributor Author

No more double 'blah' highlight :)

blah

@blowery blowery force-pushed the fix/reader-sidebar-highlight branch from 1f47b64 to 49373a8 Compare December 22, 2015 17:12
blowery added a commit that referenced this pull request Dec 22, 2015
@blowery blowery merged commit 20874ef into master Dec 22, 2015
@blowery blowery deleted the fix/reader-sidebar-highlight branch December 22, 2015 17:23
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 22, 2015
@bluefuton
Copy link
Contributor Author

Thanks @blowery!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Reader The reader site on Calypso. [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reader sidebar: list highlight can affect several lists with a common start to their URL
3 participants