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

Rework main navbar #1769

Merged
merged 10 commits into from
Nov 4, 2021
Merged

Conversation

liquid-flow
Copy link
Contributor

@liquid-flow liquid-flow commented Sep 25, 2021

Scope

As a right-handed mobile app user
I want to be able to navigate the app in portrait mode using my thumb effortlessly
So that I will save my thumb from stretching to the upper-left corner of my phone

This is an attempt to make a step towards mobile-first design, as described in this article.

Main concept

follow-the-thumb

This PR:

  1. Turns the menu vertical list into responsive finger-friendly tiles
  2. Pushes the hamburger menu toggle to the end of the bar
  3. Sticks the navbar to the bottom for mobile devices in portrait mode
  4. Adds a close icon for the toggler
  5. Enables displaying of the "Donate" text starting on 576+
  6. Removes the favicon
  7. Fixes navbar items vertical alignment and spacing
  8. Introduces some minor refactoring of the component

How to test

  1. Run make ui-start and navigate to http://localhost:3000
  2. Play with the navbar on mobile resolutions (<576px)
  3. Check the navbar on other breakpoints: 576, 768, 992, and 1200px

Screenshots

320x600

Before After
320x600-z75-before 320x600-z75-after

576x750

Before After
576x750-z75-before 576x750-z75-after

768x1000

Before After
768x1000-z50-before 768x1000-z50-after

992x700

Before After
992x700-z75-before 992x700-z75-after

1200x700

Before After
1200x700-z75-before 1200x700-z75-after

@liquid-flow liquid-flow force-pushed the rework-main-navbar branch 3 times, most recently from cc77021 to 4701e18 Compare September 25, 2021 18:07
@liquid-flow liquid-flow marked this pull request as ready for review September 25, 2021 18:08
Copy link
Contributor

@gitgiggety gitgiggety left a comment

Choose a reason for hiding this comment

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

In general I like the idea.

But the current implementation does sometimes show the menu button overlapping some content. So IMO that means that pages should leave X pixels room "whitespace" at the bottom so that when you scroll down the bottom of the actual content will be shown above the menu button. For example on the overview pages in grid mode it does overlap with the bottom of the last card and it's shown next to the navigation items (which might be buggy too when the navigation items get wider?). But in wall mode it does actually overlap part of the thumbnail image as well (especially when no navigation is shown). And, minor detail, with the menu being opened it does even overlap the "Tags" item / button (not the actual text nor image, but the clickable area).

There is also an issue with the "list" view (of scenes & performers). These table are much wider than a mobile screen which means you can scroll from left to right as well. But in these views the button mostly isn't shown at all (at least not in emulation), sometimes it is after scrolling up a bit again, but mostly it isn't.

And a minor detail. Could the button be shown with an X icon or something alike when the menu is open? To make it clear it is the close button :)

ui/v2.5/src/index.scss Outdated Show resolved Hide resolved
@liquid-flow
Copy link
Contributor Author

liquid-flow commented Sep 26, 2021

@gitgiggety I agree with your considerations, it really clashes with the content. Instead of giving that toggler a dedicated space at the bottom, wouldn't it make sense to move the whole navbar there? It's anyway easier to reach the buttons with a thumb.

Eventually we could also stick the filters to the bottom.

I updated the PR description and the screenshots. Can you please have a look when you have time?

@kermieisinthehouse
Copy link
Collaborator

Just pulled this down for testing, and I have to disagree with the latest change. I rather liked the layout @ 4701e18, I wouldn't have changed it from there. It's very strange to not have anything on the top on mobile, and the FAB was the best solution to exposing the tabs outside of the taskbar. I also wouldn't change the location of filters just yet.

One note: The icons and text and not quite vertically aligned in the pop-up menu on mobile.

@kermieisinthehouse kermieisinthehouse added the ui Issues related to UI label Oct 3, 2021
@liquid-flow
Copy link
Contributor Author

@kermieisinthehouse There's a good article in the Smashing Magazine about the bottom navigation pattern. Mobile apps utilize this pattern for years. Reason is reducing the interaction cost. Take Uber, for example.

I took the idea from my personal experience, and it proves to lessen the pain when you need to navigate, be it tabs or the settings. This should get us closer to having Stash as a PWA. Further on, we can also think on integrating the filter as a part of the menu, as a second layer, also from the bottom.

FAB is a bit noisy, it overlaps the content and generally steals space from the viewport. If we want to add filtering in the future, it will also need some space - and we can't just add a second FAB. Moving the whole navbar will kill two birds with one stone.

WDYT?

cc @gitgiggety

@WithoutPants
Copy link
Collaborator

Personally, I think this looks fantastic.

I think bottom navbar for mobile is important - and seems to be the majority view in #1728.

I wasn't able to see the previous version due to the force pushing - and I don't know what FAB is referring to.

My main feedback would be:

The bottom navbar is very crowded when authentication is enabled and the New button is shown:

image

It is also filled with items that are likely to be less frequently used than the items in the overflow menu - specifically the settings, donate, and log out buttons, and possibly even the manual. Additionally, I could see someone easily accidentally logging out with the button in this position.

I would suggest moving these items into a smaller row within the overflow menu - possibly on the top rather than the bottom in the mobile view? The main concern is getting it out of the main navbar.

@WithoutPants WithoutPants added the improvement Something needed tweaking. label Oct 20, 2021
@WithoutPants WithoutPants added this to the Version 0.11.0 milestone Oct 20, 2021
@kermieisinthehouse
Copy link
Collaborator

So I managed to recover the changes that were force-pushed using some sleuthing.

Here is what it looked like, which I strongly prefer to the current state:

Screenshot_20211022-201519
Screenshot_20211022-201543

The common items for navigation are all on the bottom, while the uncommon items like settings and help remain on the top. It also isn't as jarring because there is still a header bar, despite most of the navigation being moved.

The actual code is available in https://github.com/kermieisinthehouse/stash/tree/navbar-concept-fab

@WithoutPants
Copy link
Collaborator

I strongly prefer the bottom navbar alternative. As @liquid-flow noted, the FAB version overlaps content, and takes up a large amount of viewport space. Additionally, it seems like a likely target of mis-hits. Finally, it is redundant if we also have a top navbar.

@WithoutPants
Copy link
Collaborator

image

Here is what I've come up with. I could potentially add this in a separate PR.

@fustios
Copy link

fustios commented Oct 28, 2021

This looks good, but would it be possible to pin a few of the items from the overflow menu to the navbar as long as the menu is closed?
Maybe combining it with the already existing interface settings. Instead of just showing or hiding them they could be sortable and the first few could be shown in the navbar when the menu is closed.

image

image

@WithoutPants
Copy link
Collaborator

This looks good, but would it be possible to pin a few of the items from the overflow menu to the navbar as long as the menu is closed? Maybe combining it with the already existing interface settings. Instead of just showing or hiding them they could be sortable and the first few could be shown in the navbar when the menu is closed.

This is out of scope for this PR. Perhaps consider raising a feature request once this is merged.

@WithoutPants
Copy link
Collaborator

I've moved the utility buttons into the collapse menu. I'm going to merge this since it's a significant improvement on the existing navbar, and we can iterate further if necessary.

@kermieisinthehouse
Copy link
Collaborator

Latest changes tested. The button on the right is partially cut off by rounded corners on my device, but otherwise all looks good. It's also a little hard to hit while holding the device.

Perhaps centering it?

@WithoutPants WithoutPants merged commit 3671388 into stashapp:develop Nov 4, 2021
@kermieisinthehouse kermieisinthehouse mentioned this pull request Dec 29, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Something needed tweaking. ui Issues related to UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants