artsy / palette

Artsy's design system
https://palette-storybook.artsy.net/
MIT License
214 stars 45 forks source link

fix(EMI-1560): Add margin-top to elements when the title is present #1334

Closed MrSltun closed 11 months ago

MrSltun commented 11 months ago

This PR resolves EMI-1560

Description

This PR adds a margin-top to the input elements when the title is present to prevent the title from overlapping on other elements

Demo & Screenshots

Before After
Input
TextArea image image
Select image image
MultiSelect image image
dzucconi commented 11 months ago

I don't think we should be doing this and rather should be fixing it at the call sites.

dzucconi commented 11 months ago

To be clear why: This change would make it very confusing to ever use these inputs with layouts that have flex or grid gaps.

damassi commented 11 months ago

@dzucconi - Lets deal with this grid gap issue when it arises and get this shipped. I understand the no external margin rule, and really try to follow it, but in this case, where you have inputs on a page (typically many, back to back) this fix feels fine. The "external margin" is actually the fully-measured distance of the animated label. So in a sense its still internal.

To fix all of this at the call-site areas feels a bit much. To refactor this later would be trivial. So lets ship this thing. We can debate this elsewhere, but for now going to merge this. Feel free to open a PR fixing things you feel need addressing and making it perfect after this goes live.

dzucconi commented 11 months ago

IMO this is going to be way more annoying to work around than to just fix the problems where they are. I don't even understand how this was a problem since there wasn't external margins in the first place.

damassi commented 11 months ago

I imagine it has something to do with the animated label being pulled outside of the boundary. So while it didn't have an external margin before, with the label, it does (kinda sorta). And now we need to account for things.

We're so close to launching this. Lets work together here. Lets get the remaining issue in the QA board in and merged and we can all follow up where we see fit. Its the nature of iterating.

dzucconi commented 11 months ago

It's fine but at the very least: how do I disable this so when I work with it I can have layouts that don't have illegal whitespace values?

dzucconi commented 11 months ago

Also: nature of iterating this isn't. It's introducing a breaking visual change that's going to require constant workarounds.

dzucconi commented 11 months ago

I also wonder if this is why in Material 3 they went with a bounding box around the whole thing.

damassi commented 11 months ago

It's fine but at the very least: how do I disable this so when I work with it I can have layouts that don't have illegal whitespace values?

Open a PR real quick with a prop. If you end up not needing to use the prop before you get around to refactoring every single input callsite across force, volt, forque, (what else?) first, just open a PR and delete it.

For large last minute requests like this, feel free to take the torch and open some PRs. Its a lot to drop on someone not yourself. Feel free to ping me in slack and we can continue debating.

damassi commented 11 months ago

Slack thread continues here

artsyit commented 11 months ago

:rocket: PR was released in @artsy/palette-charts@36.0.0, @artsy/palette@37.0.0 :rocket: