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

Refactor settings to their own tabs and routes #4489

Merged
merged 6 commits into from
May 1, 2022

Conversation

MaxLeiter
Copy link
Member

@MaxLeiter MaxLeiter commented Feb 19, 2022

This seems pretty controversial from some discussion on IRC, but I like it so I'll leave it up for discussion here before closing it.

See #4489 (comment) for updated screenshots

screenshots:
image image
image image

I also split each tab into its own component and route, as the Settings.vue file is 600+ lines long.

While the settings aren't too complex right now, this allows us more flexibility to add more without worrying about clutter + we can add more tabs for features like custom keybinds.

@MaxLeiter MaxLeiter added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Feb 19, 2022
@itsjohncs
Copy link
Member

I like the use of text + icons much more than just icons. It is less aesthetically pleasing but (I think) much more useable. We can do some more passes over the aesthetics after landing too.

As I said on IRC, I don't think the settings page has enough in it to benefit from this. Since separating them out into separate pages that you need to explore makes them slower to look through I think this isn't a net positive. On a meaty desktop or tablet screen I feel like this is particularly true.

I'm also hesitant to encourage the proliferation of settings since I think each additional setting comes with drawbacks that need to be weighed.

But all that said, I don't really have a big problem with this change and it seemed like a few people were excited about it at least. So I won't fuss if people generally want to go through with it.

Making all the pages visible at once (perhaps allowing quick jumping between them using a sidebar) on large screens would alleviate my first concern too. That could be done after this change lands easily enough.

I can do a proper review after we arrive at consensus about what we're doing, but some quick thoughts I wanted to jot down so I don't lose them:

  • Autocomplete doesn't seem like it belongs under appearance.
  • "Account" might be a more widely-intuitive category title than "User Settings".

@MaxLeiter
Copy link
Member Author

MaxLeiter commented Feb 20, 2022

@itsjohncs:

...separating them out into separate pages that you need to explore makes them slower to look through I think this isn't a net positive

This is true if you know what you're looking for, but I'd argue the tabbed system is easier for newer users. Four options is not that many to consider, I think it's pretty obvious what setting should be where. Scrolling through and reading a long list (and I do feel like our settings page is long) is a lot of work.

I'm also hesitant to encourage the proliferation of settings since I think each additional setting comes with drawbacks that need to be weighed.

Agreed, but I do think there are valuable options this allows us to now explore, like admin settings, upload management, etc.

Autocomplete doesn't seem like it belongs under appearance.

This isn't really new -- autocomplete has always been under the "Visual aids" section. Where do you think it should be instead? General?

"Account" might be a more widely-intuitive category title than "User Settings".

Thanks, I was struggling to think of a better name last night 😅

@MaxLeiter
Copy link
Member Author

screenshot showing the new 'account' setting instead of 'user settings'

@MaxLeiter
Copy link
Member Author

MaxLeiter commented Feb 20, 2022

Worth pointing out that #85 and @astorije's (5 year old) mock-up of better settings exists at https://fishmonger-christopher-73101.netlify.app/.

@GewoonYorick
Copy link

Very welcome change, Opens up the door to a lot of new possibilities and gets rid of long list of settings.

Personally I feel like it'd be nicer to have the navigator on the side (and not ontop) on wider screens atleast.

Currently the navigator (selected item) looks a little off on the default theme:
image

@MaxLeiter
Copy link
Member Author

@GewoonYorick I hadn't tested on non-morning yet, oops. Thanks!

I'll play around with larger screens, but the issue is that it breaks our consistency with other pages (connect and help)

@MaxLeiter
Copy link
Member Author

MaxLeiter commented Feb 21, 2022

Updated to @GewoonYorick's suggestion of nav on the side on desktop + fixed the background color

Screenshots (click to expand) image image

@aab12345
Copy link
Contributor

Side menu much better, need to see what it looks like on a mobile with there been less space to work with.

@brunnre8 brunnre8 added the Status: WIP Work in progress or feature not fully fleshed out yet label Mar 9, 2022
@MaxLeiter MaxLeiter added Status: needs-review PR not yet reviewed by enough maintainers and removed Status: WIP Work in progress or feature not fully fleshed out yet labels Apr 18, 2022
Copy link
Member

@itsjohncs itsjohncs left a comment

Choose a reason for hiding this comment

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

Haha the changed files view on GitHub scared me. Fortunately most of the lines have just been moved from one file to another. I used this to get a nicer diff:

git diff $(git merge-base HEAD master) --color-moved=plain --color-moved-ws=ignore-space-change

There's not much actual new/changed code in here which is great. I think we could probably spend some time polishing the styling (like on mobile there's not enough to contrast the tabs from the rest of the content so they're easy to miss), but I think it's worthwhile to just push this through and do any polishing as much-much smaller PRs.

I only found one thing that needs fixing. I didn't do super extensive testing on this but I'm hoping that since very little actual JS has changed we won't have any nasty surprises.

client/components/Settings/Navigation.vue Outdated Show resolved Hide resolved
@itsjohncs itsjohncs added Status: changes-requested Review happened but some changes need to be made and removed Status: needs-review PR not yet reviewed by enough maintainers labels Apr 19, 2022
@itsjohncs itsjohncs self-assigned this Apr 19, 2022
@MaxLeiter MaxLeiter added this to the 4.3.2 milestone Apr 30, 2022
Copy link
Member

@itsjohncs itsjohncs left a comment

Choose a reason for hiding this comment

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

LGTM!

@itsjohncs itsjohncs removed the Status: changes-requested Review happened but some changes need to be made label May 1, 2022
@itsjohncs itsjohncs merged commit 5f7acbf into master May 1, 2022
@itsjohncs itsjohncs deleted the maxleiter/tabbedSettings branch May 1, 2022 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants