-
Notifications
You must be signed in to change notification settings - Fork 888
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
Make layout full-width #5294
base: development
Are you sure you want to change the base?
Make layout full-width #5294
Conversation
With 2K monitor I think limiting no. of columns to a fixed value on a wide screen makes things appearing too large Original size is 262px and now it's doubled From #5007
I suspect limiting the no. of columns is making this worse? |
Is the suggestion to set it to 6? This default was inspired from Piped, which uses 5. Invidious uses 4. YT uses 6, albeit with a secondary rule of capping the video thumbnail width at 500px, which is only reached on viewports greater than 3200px in width.
Worse how? This behavior was specifically requested by a teammate, who said that they did not like this original feature suggestion because it did not have this behavior implemented. Using larger visual real estate to fit in an arbitrarily large number of columns of videos is a worse and more overwhelming UX. |
This PR reduces the limit on the number of videos that can be displayed At least I disagree this closes #5007, whether piped design should adopted is another thing |
I respect your opinion on a lot of things, Pika, but this is the behavior of every major video app, not just Piped. I'm open to leaving that part of the PR into a separate one if it's requested, but I'm only bundling it in here directly because a teammate explicitly communicated to me that the absence of that feature was what made the previous version of the full-width layout PR an undesirable user experience. I'll check with the issue author on whether they think it resolves the issue on paper, but ultimately, I care about improving the UX more than anything else. |
Hi @deepspaceaxolotl, does this PR adequately address your original issue? The main point of contention is whether it meets your intended definition if it also caps the max number of grid columns to 5 at large viewport sizes. This is behavior adopted from other major video apps to prevent an overload of information:
|
5 columns should be enough for most screens! My main concern was getting to 4-5 columns, so this should work well, thank you everyone! |
I am fine with #5007 as the issue opener agrees |
I cant make it out from the screenshots but does this fix #4535 ? |
No, it does not |
Pending reviews from others especially on mobile side |
I should have included this in my design. I missed this issue, because I don't have a big enough screen. This looks good, but in list view, the "More Options" menu button should be located right where video description ends just like on YouTube. Currently it's a big distance to travel with the mouse. So I guess the description's width should be limited. |
Yes, sorry, I made a mistake. The grid view is perfect. |
6b0d9fb
to
83e79e1
Compare
Done. The right panel should be fixed now too. |
I tested YouTube a little bit and wrote down what they do in case you prefer to copy their behaviour (kinda like the previous solution did) instead of having these scrollbars. YouTube:
A compromise could be to cut off playlist title (maybe show the full title on hover at least? won't work for mobile users, but it's something), but have a scrollbar for the description just like we do for video description on watch-video page. I suspect that it might be possible to get rid of the third scrollbar (for the panel itself) by rearranging things in the template and changing some styles. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@kommunarr @pkrasicki are there still things that need to be done? |
@efb4f5ff-1298-471a-8973-3d47447115dc We only need approval/feedback from the team. When we fix merge conflicts it will be ready to merge. |
Conflicts have to be resolved first and merging with dev branch before we can give it some proper feedback. Some pages have been changed and that is not clear how those pages are affected by this (e.g. settings page) |
Head branch was pushed to by a user without write access
@efb4f5ff-1298-471a-8973-3d47447115dc Done. Please remove the WIP label. The settings page continues to work fine for me (the new settings were already implemented when I worked on those fixes). |
The conflicts arent resolved? |
Please revert the latest commit |
This reverts commit bfde267.
Head branch was pushed to by a user without write access
Edit: fixed it now, I must have done something wrong last time. |
…into feat/add-full-width-layout
Head branch was pushed to by a user without write access
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Button placement is off VirtualBoxVM_kNdc0VV0vO.mp4 |
Head branch was pushed to by a user without write access
Fixed. But I'm giving up on making that work with very long channel names. |
To me this underlining looks gigantic now. Maybe we should do the same underlining as the subscription page tabs? In List mode it all looks centered now but i expected it to be at the left side like so how it currently is being displayed. Maybe it being centered is a good thing? idk tbh VirtualBoxVM_xLWSVP2tSj.mp4 |
Tabs should certainly look the same on every page. The tabs on Trending page even have a different font size now (also they're not responsive). I don't understand how those things keep happening. I think the underline should be the same as on the Subscriptions page. I would also consider shrinking them, so that people can switch between them quicker and without having to move their mouse so much. Also change the inactive tab's text color to make it more visible. But that's for a different discussion/PR. As for the centered list that was @kommunarr's decision. I think it looks a little weird, especially on the Trending page with its full width tabs. But a non centered one would look a little weird too. So I guess list view is just a weird thing in general. YouTube has both, so you can see for yourself: |
Make layout full-width
Pull Request Type
Related issue
closes #5007
closes #5296
Description
Co-authored-by: @pkrasicki
(reusing some of the work from #4379)
Summary:
Note: will have to revalidate the Settings page if #5029 is merged first
Screenshots
List view, large screen:
Grid view, large screen:
Grid view, normal screen:
Testing
Desktop
Additional context
We have so many different
card
classes and styling rules throughout the app, mostly just to setmargin-inline: auto
andmargin-block: 0 60px
ormargin-block: 0 15px
. This is a potential future cleanup opportunity (i.e., including these into the default styling of theft-card
component, or setting atheme
variable based on which version we want to invoke).