-
Notifications
You must be signed in to change notification settings - Fork 151
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
CSS: standardize identity icon display #1519
Conversation
Addresses inveniosoftware/invenio-theme#282 |
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.
Looks good! Just a tiny comment/question :)
@@ -143,9 +143,9 @@ export const RDMRecordResultsListItem = ({ result, index }) => { | |||
<Item.Header as="h2"> | |||
<a href={viewLink}>{title}</a> | |||
</Item.Header> | |||
<Item.Meta className="creatibutors"> | |||
<Item className="creatibutors"> |
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.
Why was the .Meta
removed? It seems to remove the top margin if changed to just Item
?
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.
same question
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.
It adds the meta and ui classes, and some styles cascading to the children that aren't present for creatibutors lists elsewhere. To keep it, we could override or otherwise take them into account, and then add "Meta" wherever there is a creatibutor list.
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.
in this case I would suggest to try this
<Item className="creatibutors"> | |
<Item.Meta> | |
<div className="creatibutors" |
@@ -3,7 +3,8 @@ | |||
***********************************************/ | |||
|
|||
.inline-id-icon { | |||
height: @fontSizeBase; | |||
height: 1rem; |
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.
would it make sense to rename the class to open-id-branding
(and all it's usages) as we discussed in the chat?
@lhenze I'm closing this PR and taking it here https://github.com/inveniosoftware/invenio-app-rdm/pull/1641/files because it needed a rebase and I did not want to push force to you branch, just in case 😇 . You are still the author of the commit, just being merged in another PR. |
Description
Small CSS changes to standardize displays of creatibutors' identity icons between views -- landing page view, public search results view, and dashboard view. This affects the spacing between the name and the icon, the spacing between the name-icon groups, and the size of the icons.
Checklist