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 margin to media grid #2964

Merged
merged 3 commits into from
Aug 17, 2023
Merged

Conversation

DoggoOfSpeed
Copy link
Contributor

Changes
image

It has always annoyed me how the grid and the title are not aligned so this PR matches their margin values

@nielsvanvelzen
Copy link
Member

Thanks for the pull request! There are some issues that need fixing:

  • When scrolling the grid the items now have a weird cutoff at the margin instead of the edge of the screen
  • This breaks the vertical grid direction (now needs scroll both horizontally and vertically)
  • You should use display pixels (dp) in layout and scalable pixels (sp) for text

@DoggoOfSpeed
Copy link
Contributor Author

DoggoOfSpeed commented Aug 14, 2023

Yay! Now it works! I made it so that the start of the grid is aligned to the title and the end to the clock, so the value is no longer hard coded.

Also, there is no cutoff anymore.

Also also all the code changes are in the horizontal grid logic so there should be no problems in the vertical one.

Also also also I moved the title a bit to the left and moved the options buttons so that they are aligned with the clock.

And final also: maybe the code changes should be moved to setAutoCardGridValues? I'm a baby when it comes to Android programming ;/

@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Aug 17, 2023
@nielsvanvelzen nielsvanvelzen added this to the v0.16.0 milestone Aug 17, 2023
@nielsvanvelzen
Copy link
Member

maybe the code changes should be moved to setAutoCardGridValues? I'm a baby when it comes to Android programming ;/

Unfortunately the code for the grid view is already messy so there's not really a "best" place for these small changes until everything is cleaned first.

Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears to work fine now! The changed alignment of the title also looks a lot better. Thanks for the PR :)

@nielsvanvelzen nielsvanvelzen merged commit 835d052 into jellyfin:master Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants