Doist / reactist

Open source React components made with ❤️ by Doist
http://doist.github.io/reactist
MIT License
247 stars 21 forks source link

style: Apply a large right-padding to ModalHeader to match left side #789

Closed nats12 closed 1 year ago

nats12 commented 1 year ago

Short description

Recently after working on the edit filter modal, it became obvious that the right hand side padding of the modal's header was much smaller, especially when our modal body's padding is set to large.

CleanShot 2023-07-05 at 10 24 00

I see we've done it to account for button padding, but have also witnessed this style being overriden in some cases in todoist to align it with the left i.e. task detail header.

I feel it would look better if we align these at the root component, and also that it's not necessarily this component's job to account for potentially large buttons within it. Instead, it should be handled by consumers if need be. Happy to discuss this further if there are any strong opinions on this change though.

PR Checklist

Versioning

Small fix, bumped tov21.1.1

nats12 commented 1 year ago

Looping in @gnapse too as design system DRD.

scottlovegrove commented 1 year ago

Unless this is needed urgently, I would recommend waiting for @pawelgrimm as he is working on something in the same area that could fix this as part of the overall changes he's making.

gnapse commented 1 year ago

The reason I recall for the right-side padding to be smaller is to give the impression that the close button icon's is right-aligned. It is better explained with a couple of screenshots:

CleanShot 2023-07-05 at 09 25 29@2x

See how we want the icon inside the button to be the thing that is right-aligned with the modal's content. However, the button element is bigger than the button icon, as can be seen in the following screenshot:

CleanShot 2023-07-05 at 09 26 26@2x

However, that being said, I agree that hard-coding a smaller spacing to the right of the header might not be the best solution. But whatever other solution we find, it should keep in mind this need for alignment with the modal's close button icon.

cc @pawelgrimm to keep it in mind on that pull request that you're working on in Reactist.

nats12 commented 1 year ago

Unless this is needed urgently

It is not, so I will close this PR as it would be superseded by the changes @pawelgrimm is working on.

Thank you both for the context @scottlovegrove, @gnapse 👍🏼

pawelgrimm commented 1 year ago

@nats12 Feel free to commandeer my PR: https://github.com/Doist/reactist/pull/784 I'd like to finish it, but it might be a low priority for me for a while.