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

UI: Move section navigation to sidebar #5744

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

dennisreimann
Copy link
Member

@dennisreimann dennisreimann commented Feb 9, 2024

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

grafik

Mobile

grafik

@dennisreimann dennisreimann added Enhancement Improvements to an existing feature UI / UX Front-end issues, for front-end designers labels Feb 9, 2024
@dstrukt
Copy link
Member

dstrukt commented Feb 9, 2024

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

  • The sub-navigation (assuming the parent has any sub-nav items) should not be active if the parent Wallet, Settings, etc.. is not active:
Screen Shot 2024-02-09 at 10 13 40 AM

2

  • Clicking on the parent Settings will land you on the first item in the sub-navigation list:
  • Which will allow us to keep the main CTA(s) consistent at the top right:
Screen Shot 2024-02-09 at 10 29 53 AM

3

  • Then you navigate to one of the subpages, and the same main CTA is in the top right corner:
Screen Shot 2024-02-09 at 10 16 25 AM

4

  • Then you navigate a page deeper, with the CTAs still in the right, just a few more / different:
  • The only additional consideration here is do we keep the parent item active as well (colors in the sidebar), or just the current page?
Screen Shot 2024-02-09 at 10 31 50 AM

5

  • One thing to note is the padding/margins, if no sub-navigation, the margin below is only 24px or 32px (the height of the header is 152px for example). If sub-navigation exists, the margin below is 16px to ensure a little bit of spacing, if nothing additional is added. We could ensure that everything has a default margin below this case if we also bump it up to 24px (the height of the header then being 182px). The only thing to consider here is scrolling, these top sections should stay fixed, like our other views to ensure consistency. I believe you'll notice these details and adjust once implemented:
Screen Shot 2024-02-09 at 10 29 42 AM

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:
#3827
#5581

@dennisreimann
Copy link
Member Author

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
Copy link
Contributor

Tried to build but got this.
Screenshot 2024-02-10 at 16 11 50

@dennisreimann
Copy link
Member Author

@pavlenex try a dotnet clean && dotnet restore and then do another build. This looks like there's something cached and I changed those methods.

@pavlenex
Copy link
Contributor

pavlenex commented Feb 10, 2024

dotnet clean && dotnet restore

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.

@dstrukt
Copy link
Member

dstrukt commented Feb 11, 2024

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...

@dennisreimann
Copy link
Member Author

Something is broken, CI is failing too. Will fix.

@dennisreimann
Copy link
Member Author

dennisreimann commented Feb 14, 2024

  1. The only additional consideration here is do we keep the parent item active as well (colors in the sidebar), or just the current page?

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.

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!

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.

@dstrukt
Copy link
Member

dstrukt commented Feb 14, 2024

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.

Let's keep it the way it is then!

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.

Maybe we just tackle the Store Settings first in a single PR, and see how it feels for a whole section.

@dennisreimann
Copy link
Member Author

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.

@pavlenex
Copy link
Contributor

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.

@dennisreimann
Copy link
Member Author

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

@dennisreimann
Copy link
Member Author

dennisreimann commented May 22, 2024

This PR can now be user-tested on https://testing.btcpay.tech/

@pavlenex @dstrukt @ndeet @NicolasDorier @Kukks

grafik

Copy link
Member

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this quite a bit, and it's very solid. Awesome job @dennisreimann!

I still think the breadcrumbs, and the variable header pattern/component outlined in the previous comment would really round out this PR and update to its full potential, but it could also be a follow up PR, that I could also help implement, but as mentioned I might need your assistance with setting up a template @dennisreimann.

tACK

@dennisreimann
Copy link
Member Author

@dstrukt In 0f51afc I added breadcrumbs to Invoices, Forms and Roles. You can take it from here and if you want to adjust the breadcrumbs in general, see this commit in the design repo: btcpayserver/btcpayserver-design@828b7ab

grafik

@dstrukt dstrukt self-requested a review May 28, 2024 00:57
Copy link
Member

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated all of the breadcrumbs I could, found a few issues along the way:

  • Opening Manage Plugins expands the Server Settings list and should not
Screen Shot 2024-05-27 at 5 56 35 PM
  • Couldn't manage to figure out Pull Payments breadcrumbs, looks like the way we pull in the Store ID is a little unique in this view? Not sure.

@dstrukt
Copy link
Member

dstrukt commented May 28, 2024

Making changes like this:

Screen Shot 2024-05-27 at 10 15 11 PM

@dennisreimann
Copy link
Member Author

@dstrukt Thanks, I fixed the Plugins case and added the breadcrumbs to the pull payment views. Keep on going 👍

@dstrukt
Copy link
Member

dstrukt commented May 28, 2024

I'm just pushing through all of the view, and am about to have to go to bed, but documenting the issues I'm having:

  • Getting a 404 when attempting to set the 2FA breadcrumb on the Register sub-page, and not 100% sure why
Screen Shot 2024-05-28 at 12 56 38 AM
  • Similar 404 with Access Tokens
  • And the nested Create Token
Screen Shot 2024-05-28 at 1 22 57 AM Screen Shot 2024-05-28 at 1 25 28 AM
  • And Webhooks
Screen Shot 2024-05-28 at 1 29 35 AM
  • And the Payout Processors (on-chain / lightning)
Screen Shot 2024-05-28 at 1 33 01 AM

all of the breadcrumbs have been added, another sweep on your end might be helpful, but i think i got everything

@dstrukt
Copy link
Member

dstrukt commented May 29, 2024

  • Getting a 404 when attempting to set the 2FA breadcrumb on the Register sub-page, and not 100% sure why
Screen Shot 2024-05-28 at 12 56 38 AM

Fixed all but this one so far.

@dstrukt dstrukt self-requested a review May 30, 2024 18:29
Copy link
Member

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK - LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Enhancement Improvements to an existing feature UI / UX Front-end issues, for front-end designers
Projects
Status: In Review 🕵️
Development

Successfully merging this pull request may close these issues.

btn-primary /save button consistency update [Design] Improved sidebar & sub-page navigation functionality
3 participants