-
Notifications
You must be signed in to change notification settings - Fork 850
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
Conversation
Hi @pkrasicki, thanks for working on this! Would you be able to add these changes to the |
Head branch was pushed to by a user without write access
Hi @jasonhenriquez, done. I moved most of the changes to the card component. |
There was a problem hiding this 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
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.
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. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Head branch was pushed to by a user without write access
Conflicts have been resolved. A maintainer will review the pull request shortly. |
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. |
Head branch was pushed to by a user without write access
Sure, done. |
When i enable or disable this feature it closes the setting, i think that is undesired VirtualBoxVM_Wqna1MYEuc.mp4 |
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. |
Ah you're right, seems that it has to do with the FreeTube_4eIiBSrHhn.mp4 |
Blerg, gonna have to take a look at that |
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. |
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. |
@pkrasicki how would it look like with top padding? |
Welp that didnt change allot :( |
So what do we do here? I currently see 3 choices and I'm fine with either of them:
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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. |
Apologies on the long wait @pkrasicki. The team is very busy for a while now and therefore this was not prioritized. |
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. |
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. |
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. |
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. |
@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: |
Thank you! I'll see if I can apply the bits necessary for a wide layout later! |
Add setting to enable full-width layout
Pull Request Type
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
Additional context
None.