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

fix image alignment on plugin cards #2343

Merged
merged 8 commits into from
Feb 22, 2021
Merged

fix image alignment on plugin cards #2343

merged 8 commits into from
Feb 22, 2021

Conversation

dkanada
Copy link
Member

@dkanada dkanada commented Jan 24, 2021

No description provided.

@thornbill thornbill added the stable backport Backport into the next stable release label Jan 26, 2021
Copy link
Member

@thornbill thornbill left a comment

Choose a reason for hiding this comment

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

I am not a fan of the rainbow vomit introduced by adding the background color to all these cards in the dashboard.

@dkanada
Copy link
Member Author

dkanada commented Feb 12, 2021

We can always tweak the default color generation method, but the cards themselves need some kind of placeholder to differentiate the image section from the footers.

@thornbill
Copy link
Member

The addition of the background colors looks pretty terrible imo. The color is shown behind transparent user images. A large number of cards on a page becomes very overwhelming. The colors change if you leave a page and return or reload the page. And it seems they aren't applied to all cards (see the tuner cards).

Screenshot_2021-02-14 Jellyfin

Screenshot_2021-02-14 Jellyfin(1)

Screenshot_2021-02-14 Jellyfin(2)

Screenshot_2021-02-14 Jellyfin(3)

@dkanada
Copy link
Member Author

dkanada commented Feb 14, 2021

The intent was definitely to style them all the same, tuner cards were only unaffected because I didn't know they existed. I can add them as well if you'd like.

I personally love all the other images you perceived as negative. How would it sound to reduce the number of colors in the pallette or make them more similar? I will admit the brighest shade of blue is a bit too much for my taste. Regarding the transparent images, I can remove the background when an image is present to keep the old behavior.

If we're putting this much effort into the style changes I'd still like to backport them for 10.7.1 as a UX improvement, which I consider valid for backports.

@camc314
Copy link
Contributor

camc314 commented Feb 14, 2021

Don't we already use these sort of range of colors for item cards, Personally, I don't mind what we decided to do, but it wouldnt it make sense to use the same sort of colors everywhere?

@dkanada
Copy link
Member Author

dkanada commented Feb 14, 2021

We did indeed, and it would indeed, which was the idea behind this pull request. However, I'm not against tweaking the pallette used at the same time. Like I said, the brighter shades are definitely a bit in your face.

@thornbill
Copy link
Member

We do use these colors for other cards also but only ever as placeholders when an image is missing. I don’t like the choice of colors for those cases either but it’s much more aggressive having a full page of these all over the dashboard.

@dkanada
Copy link
Member Author

dkanada commented Feb 15, 2021

We shouldn't assume that every user has metadata good enough to scrape images for a majority of their media, or even that every user wants to enable images globally. For example, I use images for posters and thumbnails but prefer to leave genres and people untouched. I don't even enable metadata providers for most of my libraries. We should just design the placeholders with the assumption that they should look good even when no images are present.

Like I said, feel free to make suggestions about what colors (or color) we should use, we can always change the default theme to make them less aggresive when it's a full page of placeholders. Also, I'd like to point out that even though it's a stretch to call the device images placeholders, both the active devices and plugin cards are definitely placeholders in the proper sense, so they should be styled as such. Devices and tuners are the only two I can think of that don't really have actual images to prioritize, but I styled them to match literally every other card that exists in the client.

src/components/cardbuilder/card.css Outdated Show resolved Hide resolved
src/controllers/session/login/index.js Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Feb 17, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@thornbill thornbill left a comment

Choose a reason for hiding this comment

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

👍 lgtm

@dkanada dkanada merged commit 536797a into master Feb 22, 2021
@dkanada dkanada deleted the plugin-icon branch February 22, 2021 01:25
joshuaboniface pushed a commit that referenced this pull request Feb 28, 2021
fix image alignment on plugin cards

(cherry picked from commit 536797a)
Signed-off-by: Joshua M. Boniface <joshua@boniface.me>
@joshuaboniface joshuaboniface removed the stable backport Backport into the next stable release label Feb 28, 2021
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.

5 participants