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

Folder improvements #2979

Merged
merged 10 commits into from
Aug 29, 2023
Merged

Folder improvements #2979

merged 10 commits into from
Aug 29, 2023

Conversation

DoggoOfSpeed
Copy link
Contributor

@DoggoOfSpeed DoggoOfSpeed commented Aug 18, 2023

Changes:
Moved the folder icon to the top left for everything else (Saves space for the name)
Changed purple theme's indicator to match all other themes (Although honestly I prefer the rounded rectangle over the circle, but it was the odd one out)

@DoggoOfSpeed DoggoOfSpeed changed the title Folder spacing fixes [Draft] Folder spacing fixes Aug 18, 2023
@DoggoOfSpeed DoggoOfSpeed changed the title [Draft] Folder spacing fixes Folder spacing fixes Aug 18, 2023
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.

The icon looks a lot better on the top-left. It does look like the padding of the text is bigger on the left side compared to the right side though.

app/src/main/res/layout/view_card_legacy_image.xml Outdated Show resolved Hide resolved
app/src/main/res/values/attrs.xml Outdated Show resolved Hide resolved
android:textAlignment="center"
android:textSize="11sp"
android:visibility="invisible"
android:textSize="10sp"

Check warning

Code scanning / Android Lint

Text size is too small

Avoid using sizes smaller than 11sp: 10sp
@DoggoOfSpeed
Copy link
Contributor Author

So that's a pretty big commit 😅. Here's what changed:

Watched indicator changes:
Moved the logic that decides how/if to show the blue watched indicator to setUnwatchedCount();
Now the watched indicator is only shown on shows as it's not very useful on folders;
The settings that decide when to show the indicator have been renamed to be clearer;
The setting When the show has been watched (previously Hide unwatched count) now works.

Folder card changes:
As per your suggestion I converted both the watched indicator and the item type icon into FrameLayout;
The text margins to the side of the card have been made identical;
The number of items in the folder is now semi-transparent to help with readability as under the right circumstances a user may confuse the number as part of the folder name.

@DoggoOfSpeed DoggoOfSpeed changed the title Folder spacing fixes Folder improvements Aug 25, 2023
@nielsvanvelzen
Copy link
Member

That's indeed a big commit. Unfortunately we cannot introduce string changes now if we want to land the changes in the 0.16 release. I'm also not a fan of the unrelated changes you added in the last commits (like changing the behavior of the watch indicator), those things should be in a separate pull request.

@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Aug 26, 2023
@nielsvanvelzen
Copy link
Member

The changed color for folder item count is barely readable when the item doesn't have a dark background.

@DoggoOfSpeed
Copy link
Contributor Author

Yeah, I can see that being an issue 😅. I'll see what I can do.

@DoggoOfSpeed
Copy link
Contributor Author

Alright, so in the end I decided to just force some distance between the title and the item count as:

  1. Moving it just sounded dumb. A third circle? Really?
  2. Changing the color to non-white just didn't look very good.
  3. Changing the size of the text was also rather weird.

Now the contrast for the counter should be around the same as before the PR while the folder title is actually whiter as I removed the subtle transparency.

@nielsvanvelzen nielsvanvelzen added this to the v0.16.0 milestone Aug 29, 2023
@nielsvanvelzen
Copy link
Member

Looks fine now, thanks!

@nielsvanvelzen nielsvanvelzen merged commit 9c55464 into jellyfin:master Aug 29, 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