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

Add setting to enable full-width layout #4379

Closed

Conversation

pkrasicki
Copy link
Contributor

Add setting to enable full-width layout

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Implements #3918

Description

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

Screenshots

full-width-only2

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.

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 24, 2023 17:26
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 24, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc linked an issue Nov 24, 2023 that may be closed by this pull request
3 tasks
@kommunarr
Copy link
Collaborator

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).

auto-merge was automatically disabled November 25, 2023 12:16

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 25, 2023 12:16
@pkrasicki
Copy link
Contributor Author

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

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.

Full width and refresh button are not the best of friends

VirtualBoxVM_ywI4wRlAlv.mp4

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

VirtualBoxVM_V0erXYRdGB.mp4

@pkrasicki
Copy link
Contributor Author

pkrasicki commented Nov 25, 2023

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:

trending-without-color-changes

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.

Copy link
Contributor

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

@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 Nov 25, 2023
auto-merge was automatically disabled November 26, 2023 09:38

Head branch was pushed to by a user without write access

Copy link
Contributor

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

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 26, 2023 09:39
@absidue
Copy link
Member

absidue commented Nov 26, 2023

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.

auto-merge was automatically disabled November 26, 2023 11:51

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 26, 2023 11:51
@pkrasicki
Copy link
Contributor Author

Sure, done.

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

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

VirtualBoxVM_Wqna1MYEuc.mp4

@pkrasicki
Copy link
Contributor Author

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

FreeTube_4eIiBSrHhn.mp4

@kommunarr
Copy link
Collaborator

Blerg, gonna have to take a look at that

@absidue
Copy link
Member

absidue commented Nov 26, 2023

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
Copy link
Member

absidue commented Nov 26, 2023

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

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
Copy link
Contributor Author

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

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

@pkrasicki
Copy link
Contributor Author

pkrasicki commented Dec 22, 2023

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.

Copy link
Contributor

github-actions bot commented Jan 3, 2024

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

@pkrasicki
Copy link
Contributor Author

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.

@pkrasicki pkrasicki closed this Jan 26, 2024
auto-merge was automatically disabled January 26, 2024 18:58

Pull request was closed

@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
Copy link

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
Copy link
Collaborator

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 kommunarr reopened this Apr 23, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 23, 2024 12:36
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 23, 2024
@kommunarr kommunarr added PR closed with salvageable progress PR: WIP and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Apr 23, 2024
@kommunarr
Copy link
Collaborator

kommunarr commented Apr 23, 2024

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
Copy link
Collaborator

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.

@kommunarr kommunarr closed this Apr 23, 2024
auto-merge was automatically disabled April 23, 2024 18:14

Pull request was closed

@github-actions github-actions bot removed the PR: WIP label Apr 23, 2024
@pkrasicki
Copy link
Contributor Author

@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:

FreeTube 0 20 0 Restyled 1 0 0

FreeTube 0 20 0 Restyled 1 0 0_2

Patch:
FreeTube-0.20.0-Restyled-1.0.0.txt

@deepspaceaxolotl
Copy link

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

@kommunarr kommunarr mentioned this pull request Jun 20, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Small UI redesign proposal
5 participants