feat: removed reserved space for empty titles in vega_card #1923 #2146
Conversation
There was a problem hiding this comment.
Thanks for the valuable contribution @MattiasOz! Just need to resolve 1 comment and it's good to be merged.
ui/src/vega.tsx
Outdated
| {!title && | ||
| <div className={css.body} style={{marginTop: 0}}> | ||
| <XVegaVisualization model={{ specification, data, grammar }} /> | ||
| </div>} |
There was a problem hiding this comment.
Let's try to avoid unnecessary duplication. Do you think we could move the margin from body to title instead? That would remove the need for hacky conditional logic.
There was a problem hiding this comment.
Alright. Noticed an easier way of doing it. Updated now. Still passes all tests.
There was a problem hiding this comment.
Does the vega_card with a title look the same as before? A quick look at the code suggests the margin is missing.
There was a problem hiding this comment.
The margin is gone. We assumed it was a mistake since the other cards in the dashboard don't seem to have one. We assumed this is what the issuer was complaining about since the title doesn't actually have a reserved height if the string is empty. Our fix removes the margin altogether as well as the title div if it's empty.
We could add marginBottom to the title div if the extra spacing was necessary.
There was a problem hiding this comment.
It would be better to keep the original behavior untouched - if the title is specified, it should render as before. If not specified, there should be no extra whitespace.
Please check the rest of the components that may suffer from similar problem as well.
There was a problem hiding this comment.
We added a margin when the title isn't blank. There will be no margin when the title is absent.
The PR fulfills these requirements: (check all the apply)
mainbranch.feat: Add a button #xxx, where "xxx" is the issue number).Closes #xxx, where "xxx" is the issue number.uifolder, unit tests (make test) still pass.Closes #1923