Add setting to enable full-width layout

[pkrasicki]

Add setting to enable full-width layout

Pull Request Type

• [ ] Bugfix
• [x] Feature Implementation
• [ ] Documentation
• [ ] Other

Related issue

Implements #3918

Description

Implements a full-width layout, which can be enabled in settings. Disabled by default.

Screenshots

Testing

I tested it manually on a desktop and mobile resolution.

Desktop

• OS: Any
• OS Version: Any
• FreeTube version: 0.19.1 dev

Additional context

None.

[kommunarr]

Hi @pkrasicki, thanks for working on this! Would you be able to add these changes to the ft-card component styling, so as to prevent the need for most of the individual component-level changes? If it's the case that it only should occur to some ft-card components and not others when the setting is enabled, you can add a prop to the ft-card component outlining when to use it (or not use it if the "do not" cases are very rare).

[pkrasicki]

Hi @jasonhenriquez, done. I moved most of the changes to the card component.

[pkrasicki]

The button is in the same place as always (it's the page content that has moved), but yes, it looks weird. In my original proposal the card backgrounds were removed, so it made sense:

Perhaps we should move the button to the left and bottom (just in full-width mode), so that it fits inside of the card. I'm open to suggestions.

Maybe this is the reason that looks weird, it randomly moves up when enabled

The top padding is removed from every page on purpose. I decided to not make the settings page use full width, though. It just didn't feel right to me. But I can change it, if you think it should behave like all the other pages.

[github-actions[bot]]

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

[github-actions[bot]]

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

[absidue]

Could you please revert all your changes that converted pure CSS files to SCSS? In the files that you actually use SCSS specific features it's okay, but for all others it just adds unnecessary build steps.

[pkrasicki]

Sure, done.

[efb4f5ff-1298-471a-8973-3d47447115dc]

When i enable or disable this feature it closes the setting, i think that is undesired

https://github.com/FreeTubeApp/FreeTube/assets/73130443/77700e1a-6e7b-42af-b7f0-eb4007ed86e1

[pkrasicki]

I can reproduce it, but it happens in the development branch as well when clicking other toggles, using dropdowns and sliders. So this bug is not caused by my changes. It doesn't happen in 0.19.1 release, though.

[efb4f5ff-1298-471a-8973-3d47447115dc]

Ah you're right, seems that it has to do with the Expand all Settings Sections toggle

https://github.com/FreeTubeApp/FreeTube/assets/73130443/f98192da-a8fd-4839-b733-b1431cbe2e45

[kommunarr]

Blerg, gonna have to take a look at that

[absidue]

Toggling the switch updates the value in the db and store, which triggers the watcher that Vue placed on the getter for that value in the store, because we access it through a computed property in the component and the change makes Vue re-render/update the component. When a details element is open the browser sets the open attribute on it. During the update Vue reads the computed property for the open all sections by default, which is false if that setting is disabled, so because the component update says the section should be closed but it's currently open, Vue closes the section.

Before the open all sections by default setting was added, nothing controlled when a section was open other than the user, so those component updates weren't a problem.

The solution is probably to find a way to not have the open all sections by default setting in a computed property/find some way that it only applies when you first open the settings or when you toggle the switch.

[absidue]

Well the choice is fix that one setting or rewrite all settings sections to not use computed properties for the settings and just read the settings in the created lifecycle hook, but that will make it a lot harder for settings that depend on other to be live disabled or settings that are changed in other windows updating automatically in the current window.

[efb4f5ff-1298-471a-8973-3d47447115dc]

I dont think a rewrite is necessary because #4387 will probably remove the existence of that toggle

[absidue]

@efb4f5ff-1298-471a-8973-3d47447115dc Rewriting the settings to have tabs will likely take a while, in the mean time something will have to be done about the bug, the quickest solution would be to revert the pull request but it would be nice if there were a workaround, even if it's just temporary.

[efb4f5ff-1298-471a-8973-3d47447115dc]

The top padding is removed from every page on purpose. I decided to not make the settings page use full width, though. It just didn't feel right to me. But I can change it, if you think it should behave like all the other pages.

@pkrasicki how would it look like with top padding?

[pkrasicki]

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

[efb4f5ff-1298-471a-8973-3d47447115dc]

Welp that didnt change allot :( Lets wait for the others to chime in what to do with this.

[pkrasicki]

So what do we do here? I currently see 3 choices and I'm fine with either of them:

1. We ignore the issue and merge the changes. It's a very small problem and since it's an optional feature, most users won't be affected.
2. We decide that this is unacceptable and since I don't see a way to fix this, we ditch the proposal entirely for now.
3. We instead implement my original full-width proposal but with original colors (exactly like on the screenshot), which doesn't have this issue and works better on mobile (a bit less padding). But as @jasonhenriquez has noticed it would require making some changes to some of the themes, which should just mean replacing page bg color with old card bg color, which is easy. Making those theme changes work only when the setting is enabled might be harder, though.

[github-actions[bot]]

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

[pkrasicki]

We've been talking about this for 5 months now with very little progress. I'm gonna give up. Feel free to use the code from my branch if you want to work on this.

[efb4f5ff-1298-471a-8973-3d47447115dc]

Apologies on the long wait @pkrasicki. The team is very busy for a while now and therefore this was not prioritized.

[deepspaceaxolotl]

Is this going to be merged at some point? I find the padding in the newer versions takes up a lot of space on my relatively small screen and would like to go back to 4 videos per row instead of just 2-3, so a full-width switch would be very much appreciated.

[kommunarr]

This PR took a while because we got bogged down in details before getting consensus on how we would like this feature to look like as a team, while we were also in the midst of the user playlist push. I like this PR is fine as is, and I don't think we should have let it stay in stasis for so long. Main issue for this one is resolving the merge conflicts and fixing the appearance for playlists, which still had a sizable right margin in the original PR (I'm testing now). If we want to, we can put a tooltip clarifying it doesn't apply to the Settings page, or change the label to say "For Most Pages". I think the only thing up for debate, IMO, is whether we enable this setting by default, and/or remove the option entirely. I think it probably adds some testing burden to keep both versions alive, and I think this is a reasonable default.

[kommunarr]

Bringing up concerns from the Matrix chat, this can potentially be overwhelming at larger screen sizes. Any such implementation will need to limit the # of items displayed per grid row to a reasonable maximum (e.g., 5). This has a side effect of preventing users from having obscenely large numbers of videos displayed when the interface is zoomed out, but the app is also borderline unusable when the zoom is reduced to such an extent, so this would probably make the app much more usable at lower zoom levels. See Piped's implementation for reference.

[kommunarr]

Closing this PR so that the original author does not get unfairly pinged on this. For anyone who wants to work on this story, please do so as a new PR.

@deepspaceaxolotl Please create a new feature request specifically for this ask for tracking purposes.

[pkrasicki]

@deepspaceaxolotl Here is a patch that you can apply on top of 0.20.0 release to get my Restyled™ version of FreeTube. It's implemented almost only with CSS and it looks like this:

Patch: FreeTube-0.20.0-Restyled-1.0.0.txt

[deepspaceaxolotl]

Thank you! I'll see if I can apply the bits necessary for a wide layout later!