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(freestyle-menu): Reset f queryParam in menu #83

Merged
merged 1 commit into from
Feb 19, 2017

Conversation

fsmanuel
Copy link
Contributor

@chrislopresto First of all this addon is amazing!

The PR fixes a minor bug. You can see it in action:
http://chrislopresto.github.io/ember-freestyle/?f=foo-normal&s=Foo%20Things&ss=Foo%20Subsection%20A

If you click on an item in the navigation a blank page is rendered. The PR fixes it by reseting the f queryParam.

@chrislopresto
Copy link
Owner

Thanks! I can see how the current behavior is confusing. Though I'm not sure that clearing the filter when switching sections is what we should do. Perhaps we should add a simple visual indication that a filter is active, along with an x to reset the query param.

What do you think?

@tamzinblake
Copy link
Contributor

tamzinblake commented Feb 16, 2017

We've come across this bug too. In general, the filter doesn't make a lot of sense to me - I usually end up with that query param by clicking something that doesn't even seem like a filter, and then I'm confused when I click on a different thing and get a blank page. I'd been assuming it was something to do with our router setup but I guess it's a general problem.

I don't know if resetting the filter is always the right thing to do, but if there were an option to turn this feature on I'd happily turn it on and forget.

@tamzinblake
Copy link
Contributor

Actually I'm not sure how that feature is even supposed to work - it turns off a lot of the information on the page about other components, but not even all of it, and there isn't any button I can see to remove the query string once it's been set.

@chrislopresto
Copy link
Owner

Fair enough. If folks are confused, then resetting is the better option. Thanks!

@chrislopresto chrislopresto merged commit 6c6a822 into chrislopresto:master Feb 19, 2017
@tamzinblake
Copy link
Contributor

Thanks! Will there be a new version published soon?

@chrislopresto
Copy link
Owner

Just automated the release script and released v0.2.14

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.

3 participants