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

Adjust positions of grid UI elements #1099

Merged
merged 4 commits into from
Aug 27, 2021

Conversation

linetrimmer
Copy link
Contributor

@linetrimmer linetrimmer commented Aug 23, 2021

Changes

  • Moves the toolbar to below the clock
  • Slightly decreases the margins of the NowPlayingBug
  • Matches the end margin of the item count with the start margin of the status text
  • Moves the item count down 2sp

Screenshots (Before/After)

No clock, no now playing

before-no-clock-no-nowplaying
after-no-clock-no-nowplaying3

With clock, no now playing

before-with-clock-no-nowplaying
after-with-clock-no-nowplaying3

No clock, with now playing

before-no-clock-with-nowplaying
after-no-clock-with-nowplaying3

With clock, with now playing

before-with-clock-with-nowplaying
after-with-clock-with-nowplaying3

counter/statusText

before-counter
after-counter

Issues
Fixes #1097

@thornbill
Copy link
Member

Personally I'm not a fan of the position change of the filter toolbar for a couple reasons:

  • The filter buttons take up a lot of space that interferes with long titles.
  • It highlights the inconsistent handling of the user avatar. (Currently it is selectable on the home screen but not other screens.)

Otherwise the screen shots of the alignment fixes look great!

@linetrimmer linetrimmer marked this pull request as draft August 25, 2021 04:09
@linetrimmer
Copy link
Contributor Author

linetrimmer commented Aug 25, 2021

Swapped the toolbar and NowPlayingBug positions and reverted the toolbar size change:

Screenshots (Take 1 / Take 2)

No clock, no now playing

after-no-clock-no-nowplaying
after-no-clock-no-nowplaying3

With clock, no now playing

after-with-clock-no-nowplaying
after-with-clock-no-nowplaying3

No clock, with now playing

after-no-clock-with-nowplaying
after-no-clock-with-nowplaying3

With clock, with now playing

after-with-clock-with-nowplaying
after-with-clock-with-nowplaying3

@linetrimmer linetrimmer marked this pull request as ready for review August 25, 2021 05:03
@MrChip53
Copy link
Contributor

Oh yes I do actually like this second version more. What if the now playing bug is inline with the toolbar, positioned after the tool bar though? This would allow the title the most room I think while also not extending the header bar itself too far down. What do you think about the now playing bug @thornbill? I think it's position in version 2 here is bad but overall version 2 is better than 1.

@thornbill
Copy link
Member

I really like the second take on this! I'm sure we'll make some additional changes to the now playing stuff in the future, but I think this is great for 0.12.

One tiny thing I noticed is I think there could be slightly more padding between the two rows. They feel just a bit too close together.

@linetrimmer
Copy link
Contributor Author

linetrimmer commented Aug 26, 2021

Moved the toolbar down 4dp and the info row down 2dp. I kept going back and forth between 3dp/4dp and 1dp/2dp, so let me know if either of them is too far down (or not enough)

Take 2 / Take 3

before-take3
after-take3-2dp-4dp


What if the now playing bug is inline with the toolbar, positioned after the tool bar though?

I couldn't get the now playing bug to go after the toolbar since the toolbar doesn't have a reference point for layout_toStartOf when the bug is gone (that's not to say it can't be done though :p). Putting the bug before the toolbar looks fine, but it overlaps with the title when the focus is on it and it's below a "y" or "g". Moving the bug down to accommodate that didn't look good imo.

Now playing inline with toolbar

Screenshot_1630009562
Screenshot_1630009568
Screenshot_1630009684

@thornbill thornbill merged commit 56f3211 into jellyfin:master Aug 27, 2021
@linetrimmer linetrimmer deleted the grid-adjustments branch August 27, 2021 14:43
@nielsvanvelzen
Copy link
Member

I've noticed the now playing bug is positioned wrongly in the "smart screens" and I suspect it's caused by this PR.

@linetrimmer
Copy link
Contributor Author

I've noticed the now playing bug is positioned wrongly in the "smart screens" and I suspect it's caused by this PR.

I just compared this branch to master from before this PR was merged, and the now playing bug is in the same position in the smart screens.

This is what it looks like for me on the smart screens before and after this PR:

Smart screen (before/after)

before-smart-screen
after-smart-screen

IIRC the smart screens use the enchanced_detail_browse layout, so they wouldn't be affected by this PR.

@nielsvanvelzen
Copy link
Member

I guess it only became noticeable after this PR then haha

@thornbill
Copy link
Member

I noticed this but I think we can just remove the second line there since it's not displayed on the other screens. 🤷‍♂️

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.

Currently playing music covers library view options
4 participants