Move History and Export buttons to secondary nav #5151
Conversation
There was a problem hiding this comment.
Changes in this PR definetly make the UI cleaner and more user-friendly, especially on smaller screens. The addition of the updateSecondaryNav() function ensures the navigation stays consistent across different routes. Overall, solid improvement that enhances usability, well done.
There was a problem hiding this comment.
Other than this one comment, I have no more questions. Functionality, routing and highlighting work as intended.
On big screens edit button seems a bit lonely because of the big gap and visual difference with other menu items. I tried adding paddings and changing colors of it, but neither of them improved visual sufficiently. It's out of scope of this PR, but, in the future, it will be great if we redesign menu to make it more aligned with the new arrangement of buttons.
Also, changing Export text with the export icon, may end up gaining even more space and a cleaner visual without many menu items.
Overall, this PR fixes the issue in a clean, functional way.
| @@ -412,6 +412,9 @@ $(document).ready(function () { | |||
|
|
|||
| if (OSM.router.route(this.pathname + this.search + this.hash)) { | |||
| e.preventDefault(); | |||
| if (this.pathname !== "/directions") { | |||
There was a problem hiding this comment.
Why do we need explicit check for the directions path? If I understand correctly, this line of code is for keeping menu opened when navigating through directions pages, but app\assets\javascripts\index\directions.js:123:5 still adds "close" class to the header when it gets routes.
There was a problem hiding this comment.
The code here closes the menu after clicking on any menu item except directions. By menu I mean everything that shows up when the burger button is clicked. The directions form is inside this menu, therefore an exception is made for it, the menu should stay open to keep the form visible.
app\assets\javascripts\index\directions.js:123:5 happens after the route endpoints are received, then you can hide the form and the rest of the menu.
There was a problem hiding this comment.
app\assets\javascripts\index\search.js:13:5 sets "prevent default" to the directions button e.preventDefault() and app\assets\javascripts\index.js:399:9 checks if the <a> tag is "prevent default" e.isDefaultPrevented(). Therefore, returns empty value and never reaches this.pathname !== "/directions" if statement.
There was a problem hiding this comment.
Why should it reach this.pathname !== "/directions"?
If you're at app\assets\javascripts\index\search.js:13:5, you have the menu open. And you want it to stay open because you're switching from the search form to the directions form. if (this.pathname !== "/directions") ... closes the menu, something you don't want at this point.
There's #4854 which is slightly better in this respect, although the gap is still there. But we'll be able to have more secondary menu items after the change shown in #4854 (comment), this may make the gap smaller. |
Version of #4854 that also moves History to secondary navigation.