-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Enable stats header row wrapping and expandable counts #8343
Enable stats header row wrapping and expandable counts #8343
Conversation
<EntityTitle onClick={onClick} $titleSizePx={titleSizePx}> | ||
{name || ' '} | ||
</EntityTitle> | ||
<FirstRow> |
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.
The changes to the jsx are:
- Wraps the left/right columns in a FirstRow
- Wraps the stats (subheader) in a SecondRow
- Moves the subHeader to the SecondRow
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.
this makes sense! I'd ask that before merging we do one more manual test to ensure things look good in all the different variations where there are stats, when there's no stats, when there's tags, terms, owners, lots of them, none of them, etc.
The reason I ask is because this component is notoriously fickle and lots of things rely on other things as you can see by the messy code below here...
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.
nice stuff! a couple of minor questions and only real concern is how mixing these styles up may affect other areas (like the entity profile sidebar)
@@ -7,31 +7,34 @@ type Props = { | |||
}; | |||
|
|||
const StatsContainer = styled.div` |
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.
can we maybe add a minor padding/margin to the bottom? even like 4px or something. it looks a little tight on the bottom of the card between the stats and edge of card!
<EntityTitle onClick={onClick} $titleSizePx={titleSizePx}> | ||
{name || ' '} | ||
</EntityTitle> | ||
<FirstRow> |
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.
this makes sense! I'd ask that before merging we do one more manual test to ensure things look good in all the different variations where there are stats, when there's no stats, when there's tags, terms, owners, lots of them, none of them, etc.
The reason I ask is because this component is notoriously fickle and lots of things rely on other things as you can see by the messy code below here...
`; | ||
|
||
const DomainWrapper = styled.span` | ||
display: inline-block; | ||
margin-bottom: 8px; |
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.
I believe this DomainLink (and the DataProductLink and tags below) are all used in the sidebar on profile pages. do things still look good there with these CSS changes?
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.
yeah I think the spacing may be a little off in that sidebar. you can always wrap the components like DomainLink
in a new spacer span styled component with margin-bottom:8px;
for that specific use case! probably better there anyways than forcing styles everywhere it's used.
Stats regressed somewhat after #8232 where each column was wrapping and became hard to read. Previously, overflow content would wrap to the next line. This PR adds that behavior back in and also fixes a small bug where popovers were opening inside of a tooltip. This also expands upon the expansion functionality of the hover for more fields.
Checklist