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

Settings menu & UX improvements #5029

Open
wants to merge 26 commits into
base: development
Choose a base branch
from

Conversation

jasonhenriquez
Copy link
Collaborator

@jasonhenriquez jasonhenriquez commented Apr 27, 2024

Settings menu

Pull Request Type

  • Feature Implementation

Related issue

closes #4387

Description

  • Implements a menu in settings for more easily navigating the sections
  • Currently scrolled-to section is marked as active
  • Mobile and tablet version of settings uses "fake pagination" implementation
  • CHANGE: Removes "Settings expanded by default" setting and <details> / <summary> tags (now inapplicable to current design)
  • CHANGE: General Settings is now always the top setting, regardless of sorting preference (read comment on e9c527a)
  • CHANGE: Other settings sections style changes, including rounded corners, external section titles, and removal of <hr> tags (for a sleeker aesthetic)
  • CHANGE: Removes experimental removing of underlines on links pre-hover with the Hot Pink theme (breaks menu link styling, and was not ever noticeably beneficial)
  • CHANGE: moves settings sections from data to computed because it's needed for short title to long title fallback logic

Screenshots

Suggested Design suggested
Live Design live

Testing

  • Confirm that Settings Menu works intuitively
  • Confirm that General Settings is always at the top of the settings, even after sorting
  • Confirm that "long" version of settings label is displayed in the Settings Menu if no "short" label exists (i.e., as of now, with any non-EN-US locale)
  • Test tablet & mobile views

Desktop

  • OS: OpenSUSE
  • OS Version: TW

Additional context

Remaining action items

  • Use SponsorBlock icon instead of generic shield icon for SponsorBlock Settings (this icon is not supported by font-awesome-icon)

Aspects open for discussion
When a top banner is present on desktop view, the General Settings menu item is too low to be active, so no tab is shown as currently active if you scroll to the top. This is a very minor visual nitpick, but I just figured I would get it out of the way and say that having no active items until you scroll down a bit more in this exceptional case is fine and makes sense.

Solving the problem of preventing 'jumping around' when changing locales with alphabetical sorting is hard. This problem is easily solved by stickying general settings to the top of the list unconditionally. This is in line with ChunkyProgrammer's initial assessment of how to improve the settings order in FreeTubeApp#1739, albeit not also moving Theme Settings to the top for the time being. The rationale from a functional level is that General Settings is a hub. Even when you change languages, change sort order, or what have you, General Settings is right at the top. I don't imagine we need to update the label of the setting, as I think this relationship is quite intuitive.
@jasonhenriquez jasonhenriquez added the PR: draft awaiting team consensus For a functionally complete PR whose implementation is still under discussion or expected to change. label Apr 27, 2024

This comment was marked as resolved.

This comment was marked as outdated.

@PikachuEXE
Copy link
Collaborator

  1. Not a breaking issue but why rounded corners
    FT has square "back plates" in other places

image
image

  1. (I am not a mobile user so I am guessing here)
    The old UI is faster for user to get to lower sections (find section, open, do stuff)
    The new one requires plenty of scan time due to all expanded settings

@jasonhenriquez
Copy link
Collaborator Author

jasonhenriquez commented May 1, 2024

Not a breaking issue but why rounded corners

Short explanation: This was admittedly a subjective design choice that I think makes for a sleeker look here.

Longer explanation: In design, you want to maximize the alignment of elements, such that if you were to draw a straight horizontal or vertical line from the start of each page element, you would have page elements line up as much as possible such that there are the fewest amount of lines present. This makes visual scanning easier. And so if we have straight edges, we would be inclined to align each settings section title along the edge exactly. But having the headings non-indented also makes their importance in the visual hierarchy less clear. So rounded edges here hit a sweet spot where there's an indentation for the titles still, and also alignment with the top edge.

The old UI is faster for user to get to lower sections (find section, open, do stuff)
The new one requires plenty of scan time due to all expanded settings

Are you referring to the removal of the "Settings expanded by default" setting? That's a fair note. If Emma and/or other mobile-savvy folks think mobile navigation will be harder without that setting and without having some form of accessing the new menu, I can try to get a mobile version of the Settings menu included in this PR rather than in a later one.

@PikachuEXE
Copy link
Collaborator

I am fine with rounded edges due to your explanation

General better for desktop (me) but ya mobile side might be well...

Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment

Choose a reason for hiding this comment

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

Looks very nice IMO.

When i click on a header, it kind of centers the settings section but because of this i see a part of the settings section before this one (see video 1-4 sec). What i expected it too look like when i click on a header, see video 5-7 sec

VirtualBoxVM_UhXwZnOEqJ.mp4

@jasonhenriquez
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc This was something that took a lot of work to hit a sweet spot for. The main problems we want to solve are 1) that the active Menu section appears to be the current section you're on, 2) that clicking on a section makes it actually show as the active section, and 3) that clicking on a section actually takes you to that section. The problem with choosing the top of the viewport below the top nav is that it it doesn't work for 1, as the last section that's just peaking through the top of the screen often appears as the active one, seeming pretty odd / not useful. I tried with other solutions like the exact midpoint of the viewport being the "current section line", which has the problem of either 2 or 3. I decided on using the top 1/4th of the viewport height as the "section line", which ends up being a decent compromise for all 3. Let me know if you have any better solution in mind, but this is the best I could find.

@jasonhenriquez jasonhenriquez added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 15, 2024
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@MarmadileManteater
Copy link
Contributor

I realise this was discussed above, but I want to talk about it because it bothers me: rounded corners. I understand that there may be well founded reasons for this design. However, my counterpoint would be that this design doesn't exist in a vacuum. Next to the other views, this one is very different. There is nothing inherently wrong with the rounded corners cardless look, but it doesn't look anything like the rest of the app.

What if the settingsSection had ft-card directly under it instead?
image

@jasonhenriquez
Copy link
Collaborator Author

Thanks for the feedback, that's fair. Does this look okay now?

Screenshot_20240517_181820

@PikachuEXE
Copy link
Collaborator

One issue becomes more obvious in this PR is the inconsistent casing of the section titles (in English)image

No need to fix in this PR (can be a separate one), up to you

But also the fixed width makes the title truncated and I am not sure how troubling this would be to users with language set to one of those with longer setting section title text

e.g. español (Argentina) - Which is which?
image

Food for thought: How removing overflow-x: hidden; white-space: nowrap; looks
image
image

@jasonhenriquez
Copy link
Collaborator Author

I'm personally not as much visually a fan of the wrapping. Maybe I can make it a bit wider to accommodate these edge cases. My hope was/is that the short label translations can circumvent the issue after enough time.

@PikachuEXE
Copy link
Collaborator

Maybe use a smaller font-size? 16px:

image
image
image
image

PikachuEXE
PikachuEXE previously approved these changes May 21, 2024
@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 22, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

PikachuEXE
PikachuEXE previously approved these changes May 22, 2024
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

I'm slightly concerned that it might be easy to accidentally click the wrong link in the menu on mobile, as they are close together and fingers are usually much bigger than a mouse cursor.

src/constants.js Outdated Show resolved Hide resolved
src/renderer/views/Settings/Settings.vue Outdated Show resolved Hide resolved
src/renderer/views/Settings/Settings.vue Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Use tabs on settings page
5 participants