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

NavBar in admin #932

Merged
merged 5 commits into from
Aug 24, 2016
Merged

NavBar in admin #932

merged 5 commits into from
Aug 24, 2016

Conversation

mstriemer
Copy link
Contributor

I tried to make this reusable and break it down into many small presentation components. The NavBar in core provides the styling and structure but lets the app insert the content. If we can use this on AMO with AMO styles then we can keep the admin up-to-date with styling changes.

In the future I'd imagine us adding more components like <NavBarMenu /> to do a pop-out side menu or a user door hanger. Then apps can mix-and-match what components we'd like.

If this feels like overkill and we don't want to share that stuff I can pull this out of core as well.

search-nav-bar mov

When you go back to search it shows the last results but doesn't show q=food in the URL. I'm not sure if that's good or bad. I guess it would be nice to update the URL but I wasn't sure if that was necessary.

Fixes mozilla/addons#9777.

@muffinresearch
Copy link
Contributor

It would be nice if the search query remained in the url - are we using the correct history setup in the router?

@muffinresearch
Copy link
Contributor

r+wc

@kumar303
Copy link
Contributor

It would be a good idea to rebase this on master to check that the new lint rules aren't nagging you about anything.

@mstriemer
Copy link
Contributor Author

Since it is just linking to /search it isn't actually navigating back. I originally wanted to add a back button but there doesn't appear to be a way to tell if you're at the bottom of the stack so we can't hide the button on the first page you load and clicking it there would take you back to a different site or the new tab page.

I could check the state for a current search query and add that to the link.

@muffinresearch
Copy link
Contributor

Since it is just linking to /search it isn't actually navigating back. I originally wanted to add a back button but there doesn't appear to be a way to tell if you're at the bottom of the stack so we can't hide the button on the first page you load and clicking it there would take you back to a different site or the new tab page.

Ah I see now, I didn't look closely enough at the animation.

Feels like a good use-case for something like a breadcrumb or just an explicit <- back to search results link.

@muffinresearch
Copy link
Contributor

That said does hitting the back button do the right thing? If it does then perhaps clearing the search results is the correct behaviour here.

@mstriemer
Copy link
Contributor Author

I kind of like the search link rendering the last results. I updated the component to replace the URL with the right query and select the text in the input in componentDidMount(). Here's how it looks:

search-form-with-select mov

Unfortunately a "back to search results" link would be a little tricky. We'd need to start keeping a stack of pages the user has visited and check if the last one was search results then render the right link.

Thoughts?

@muffinresearch
Copy link
Contributor

muffinresearch commented Aug 24, 2016

Since "Search" is currently the equivalent of a homepage link for this app. It doesn't make sense to me to click that and see the last set of results, it should be a clean slate imho.

A return to search results link is probably overkill assuming the back button does indeed work as expected.

@mstriemer
Copy link
Contributor Author

This is clearing the search if the URL does not match the state now.

@muffinresearch
Copy link
Contributor

Probably an idea to rebase this against master in-case there's anything the lint updates are going to complain about.

@muffinresearch
Copy link
Contributor

lgtm otherwise.

@mstriemer mstriemer merged commit 4fa7a54 into mozilla:master Aug 24, 2016
@mstriemer mstriemer deleted the admin-nav-bar-807 branch August 24, 2016 18:37
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.

Navigation bar/back to admin search
3 participants