Skip to content

Hide "All" button if JSON flag is set #53

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

Merged
merged 1 commit into from
Dec 22, 2016

Conversation

tommcc
Copy link
Contributor

@tommcc tommcc commented Oct 30, 2016

This adds a conditional in the nav template that will hide the "All" button if the ishControlsHide.views-all flag is set. Previously, this option existed, but was not used anywhere.

Relies on a pattern-lab/patternlab-node#547 that passes this option to the template.

@bmuenzenmeyer
Copy link
Member

bmuenzenmeyer commented Oct 31, 2016

I have no problem with this - as it's got to be the one thing on the PL chrome that users cannot control, and I have heard it come up from other users too.

Typically we try to make any changes to our shared repositories in conjunction with an issue and vote on the spec. I don't think it's as critical to do sohowever since the absence of this feature should still make the ALL button render.

@dmolsen would you like a spec issue for this?

@tommcc
Copy link
Contributor Author

tommcc commented Oct 31, 2016

Thanks @bmuenzenmeyer, I'll keep that in mind for future enhancement ideas. I should probably have a copy of the PHP version running too, so I know if something I'm trying to fix has to do with the spec or just a node bug.

In this case, the value already existed in the default config files, but was unused/not referenced anywhere, so I wasn't sure it was an already-established but not-implemented feature. Hopefully I was interpreting the original intent of that flag correctly, otherwise I'm open to renaming/putting elsewhere in config (and deleting the dummy config value that started this).

@bmuenzenmeyer
Copy link
Member

@dmolsen

if i don't hear from you by 11/20 - i am going to accept this

@bmuenzenmeyer
Copy link
Member

I knew this was used before... I found an old reference to the key which you can see was a wrapper around other controls. 6bd898c#diff-d4108beafd71e30c5be9d9a05026f01dR111

Since, it has fallen into disuse.

I am in favor of repurposing it for the cause of this PR.

@bmuenzenmeyer bmuenzenmeyer merged commit 8aa20ee into pattern-lab:dev Dec 22, 2016
bmuenzenmeyer added a commit that referenced this pull request Dec 22, 2016
@bmuenzenmeyer bmuenzenmeyer mentioned this pull request Dec 22, 2016
@tommcc tommcc deleted the add-conditional-view-all branch October 30, 2017 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants