-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Rework person badges for consistency #3441
Rework person badges for consistency #3441
Conversation
For reference, we are using FontAwesome in the project. I'm am tight on time atm, but I will try to review this before I'm off for the year. |
Looks fine. Can you take out so many of the inline styles and create a class for it instead? |
Please ping me once you've done this, so I can merge into the PR Flush branch and push to the nightly branch. I'm waiting on this PR. |
@majora2007 Done! I added an objectFit attribute to the image component, too, since I noticed it was at least used on the badges in the series details page, and I figured it was better to add it to that component instead of using CSS to define that everywhere (And for consistency, I used it on the person details page too). The property defaults to "fill", which is the default in CSS, and has type info for valid settings. |
@@ -1,4 +1,19 @@ | |||
.main-container { | |||
margin-top: 10px; | |||
padding: 0 0 0 10px; | |||
|
|||
.person-badge { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last I saw on the image component, this wont take effect due to the view encapsulation. Can you validate for me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
Fixed
First part of the UI/UX changes I'm looking at doing. There's still the errorImage to take care of, but I'm thinking we should leave that as-is and maybe replace it with an SVG or something to avoid the pixelation (Along with making sure images still have the new background to ensure that error image would have it).
Otherwise, if it's possible to somehow catch the error and set HasCoverImage to false, we can just remove it completely and fallback to the icon.
As an aside, I think it'd be beneficial in the long run to look at having an icon component that ensures the icon is square and all icons have readable names when relevant (For WCAG and all that). Maybe something like this? It's not for this PR anyway, but it would make consistency easier (and probably some code cleaner) across the UI, I think.