-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
UI: Move section navigation to sidebar #5744
base: master
Are you sure you want to change the base?
Conversation
So fast! So I think most of the functionality has been covered, but just running through it myself, and on this PR for posterity and testing: 1
2
3
4
5
Finally, I'm not sure we'll want to do this for all the pages this could apply to, as this PR would become quite large, but it could be added to a section at a time (plugins, wallet, etc..) if we find we like the updated navigation structure! Can also add that it closes, assuming we do all views: |
Thanks for the detailed feedback! Yes, all of these are valid and open points. Wanted to provide it to get a better feel for this as a potential approach — there are indeed lots of details to work on of we go for it and I'd rather do it once my other PRs are done, because it would most likely resuot in larger conflicts if we do them alongside each other. So think of this in its current form more like something that'll help us evaluate this approach. I for one like the result and would +1 it. |
@pavlenex try a |
Yeah tried that multiple times, nuked everything volumes, and even folders and images, but will give it another try. Edit: hm something is wrong with this PR, let's see if others can build it I can't on Ryder. |
Will try to build it either tomorrow or Monday morning and see I run into the same errors. I've also been having trouble with some builds, dependencies, etc... |
Something is broken, CI is failing too. Will fix. |
97dcddb
to
4e62c65
Compare
I'd say only the current page — if we want to be fancy, we could also highlight the icon of the active section, but not it's associated main page name.
Yes, if we tackle everything (moving CTA's etc.) this will be a larger undertaking. Let's settle on the approach while we wait for the other open PRs to be finished, as this will touch many views and tests. |
Let's keep it the way it is then!
Maybe we just tackle the |
Given the extend of the changes I think it would be worthwhile to do this combined with #5581 once most of the other PRs are done. While this is on hold we can decide on the details. |
4e62c65
to
0b9f6b9
Compare
Gave it a quick try, must say it's growning on me, a lot of user-testing and some of the cleanups wrt settings naming should be done. Can't say for sure if this is the way to go, but the more I used it, the more I liked it, looking forward to what the team things, and then we should do some user-tetsting with community. Approach ack. |
085d219
to
bec19e8
Compare
Rebased and added the account pages. Now all pages with subnavigation are working and we can get to the details once we decide this is something for v2.0 |
@dstrukt Thanks, I fixed the Plugins case and added the breadcrumbs to the pull payment views. Keep on going 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK - LGTM!
Proof of concept for the new sidebar structure, including the active section navigation. As discussed in yesterday's design meeting — this changes the navigation for the Store Settings and Server Settings, so that we can discuss it.
I think it works very well, we'd onlty need to ensure that plugins also update/migrate.
Closes #3827. Closes #5581.
Desktop
Mobile