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

Open/close all settings tabs persistent toggle #4204

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Oct 21, 2023

Open/close all settings tabs persistent toggle

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #1743

Description

Adds toggle in Theme Settings determining whether all settings tabs or opened or closed by default. The result is immediate, but the Theme Settings tab is kept open in both cases for convenience.

Screenshot

Screenshot_20231028_114918

Testing

  • Toggle it on & see state persist across sessions, & vice versa

Desktop

  • OS: OpenSUSE Tumbleweed
  • OS Version: 2023xxxx
  • FreeTube version: 0.19.1

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 21, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 21, 2023 10:55
@absidue
Copy link
Member

absidue commented Oct 21, 2023

I wonder if it's possible to do that check inside the ft-settings-section component, instead of inside every settings section separately, that would reduce the duplicate code.

My suggested solution would be to add a prop to the ft-settings-section component so that you can still pass in a value from the theme settings one (have it optional with the default value being false), then in the created lifecyle hook set a data property based on the computed property and the prop from the theme settings. That data property can them be used in the vue template to set the value of the open attribute.

@kommunarr kommunarr force-pushed the feat/open-or-collapse-settings-toggle branch from 758b00f to 06c345b Compare October 21, 2023 14:09
@kommunarr
Copy link
Collaborator Author

Just updated now to something like that (but still uses computed to have the change reflect immediately, like we do with our other theme changes). -44 LOC; that's the benefit of getting good sleep!

static/locales/en-US.yaml Outdated Show resolved Hide resolved
src/renderer/components/theme-settings/theme-settings.vue Outdated Show resolved Hide resolved
src/renderer/store/modules/settings.js Outdated Show resolved Hide resolved
src/renderer/components/theme-settings/theme-settings.vue Outdated Show resolved Hide resolved
src/renderer/components/theme-settings/theme-settings.vue Outdated Show resolved Hide resolved
src/renderer/components/theme-settings/theme-settings.js Outdated Show resolved Hide resolved
src/renderer/components/theme-settings/theme-settings.js Outdated Show resolved Hide resolved
src/renderer/components/theme-settings/theme-settings.js Outdated Show resolved Hide resolved
src/renderer/components/theme-settings/theme-settings.js Outdated Show resolved Hide resolved
@kommunarr
Copy link
Collaborator Author

kommunarr commented Oct 22, 2023

I see examples of both expanded/collapsed & closed/open, for example in this MDN article:

This CSS creates a look similar to a tabbed interface, where activating the tab expands and opens it to reveal its contents.

This CSS creates a look similar to a tabbed interface, where clicking the tab opens it to reveal its contents.

Note the use of the language of tabs as well. Still, I changed it to your suggestions because I have no strong preference.

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Oct 22, 2023

The linked example does look like a tab
image

I would argue our settings page have sections not tabs
Tabs: Can see content of one tab at a time only (e.g browser tabs)
Sections: Can see content of none-all sections at a time

Update 1: In case someone does not know in physical world there are "folder tabs":
image

@kommunarr
Copy link
Collaborator Author

Whenever you're ready to re-review, @PikachuEXE: changes have been made.

PikachuEXE
PikachuEXE previously approved these changes Oct 26, 2023
absidue
absidue previously approved these changes Oct 27, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc

I havent tested this yet but why does the user have to go into one of the setting to find this setting feels a bit weird to me.

I would expect a toggle like this to be outside of all the settings that makes more sense in my mind

@kommunarr
Copy link
Collaborator Author

I was thinking that at first, but then again, it's a persistent setting that affects appearance of stuff, which places it in here. How would that external toggle's placement look like?

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

I was thinking of a toggle that is on top of and aligns right above the General settings

That's was my initial thought when I read the issue not sure if that makes the most sense

@kommunarr
Copy link
Collaborator Author

Here's what that would look like. It's a little too cute for me, but will go for it if others prefer it. If we do it this way, I'll remove the "By Default" text.

Screenshot_20231027_180714

@PikachuEXE
Copy link
Collaborator

So it's no longer a setting?
Just a UI component to expand all?

@kommunarr
Copy link
Collaborator Author

Oh, to be clear, I was still thinking of it as a persistent setting in that case. I just think there's less need to call attention to that fact if we change the placement, if that makes sense.

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

Here's what that would look like. It's a little too cute for me

Looks a bit weird to me, would aligning it to the left make it better?

@absidue
Copy link
Member

absidue commented Oct 28, 2023

If you put the A-Z switch from #4231 next to it, it might look better, as then there are two switches using the space instead of just one.

@kommunarr kommunarr dismissed stale reviews from absidue and PikachuEXE via 39ced66 October 28, 2023 16:49
@kommunarr
Copy link
Collaborator Author

Here's what that would look like. It's a little too cute for me

Looks a bit weird to me, would aligning it to the left make it better?

Here's what it looks like now:
Screenshot_20231028_114918

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

Looks better than placement in the middle but i would like the others to chime in also on this

  1. Yes i agree it looks weird to have a single toggle outside all of the settings but in my mind at least it makes sense to have it outside

  2. On the other hand i fully understand than if u have to put the toggle under one of the settings it would be theme settings.

In all my life i just cant remember seeing such a toggle within the menu instead of outside of all the menus but if that is totally a thing maybe it would make more sense to go with nr 2

@kommunarr
Copy link
Collaborator Author

One point in favor of @absidue's suggestion is that both of those controls change the verticality of where the Theme Settings are, so you have to scroll back to find them. Keeping them on the outside makes them very easy to toggle on/off.

@kommunarr
Copy link
Collaborator Author

I think we should merge this one before #4231. Once this is merged, I'll move the settings sort in that PR to the top area as a toggle. As stated, much better user experience for those when the settings toggles are in one fixed space as opposed to having the manipulated section bouncing to a different height.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@FreeTubeBot FreeTubeBot merged commit 62903ce into FreeTubeApp:development Nov 25, 2023
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 25, 2023
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.

Add "Open All" button to settings page to open all tabs
5 participants