refactor: Rework modal padding #784
Conversation
We consolidate outermost modal padding concerns to the high-level `Modal` component. Then, Modal composables, like `ModalHeader`, `ModalBody`, and `ModalFooter` don't need to be concerned with their x-padding. Finally, by leveraging the `gap` prop of the `Modal` component, we can remove most y-padding concerns from the composables (except for `ModalFooter`, as it requires extra space above it).
|
Ah, this change prevents dividers from taking up the whole modal width 🤦 |
First, how did you realize of the issue? Was it the Chromatic visual diff tool? Or something else? About that dividers issue. That's one reason why there's not modal-level padding. Another reason is that there may be legitimate use cases for not having the padding be applied to the entire modal. For instance, different modal layouts like the settings modal:
We still use I can also mention this comment #789 (comment). However, I'm curious about why you wanted to do this in the first place. What's the problem you're trying to solve here? |
I was just flipping through Storybook and noticed that some of the examples looked off.
The main issue is that the default amount of padding in the Reactist modal components doesn't match that of examples in the Design system and the only way to get our modals to align is to use
|
I may be seeing something wrongly, but from those 3 screenshots, the one on the left (actual implementation as seen in Todoist) and the one and the right (Figma source of truth) look the most alike. While the one in the middle, from this PR is the one that I find different from the other two (and I'm not talking about the primary button color). |
@gnapse It feels like we're looking at completely different images, because I completely disagree 😅 If you only look at differences in vertical spacing (the main concern of this PR), the middle image looks closer to the Figma mock, right? Either way, there are some issues with this PR (hence it's a draft) and I think we need to discuss further before implementing a change like this (if we decide it's needed). For now, let's just merge this additional story that attempts to implement the confirmation modal mock using the |
I agree. But only if we refer to the inner vertical spacing of the modal body. Not the outer spacing towards the edges of the modal. Which is what I thought this PR was mostly about, since it looks like it wanted to move the padding to be owned by the outer modal container element, instead of having each section have its own padding. The one in the middle, on the other hand, features the misaligned close button icon, which is what I'm mostly looking into. While also seeing almost no meaningful different in the spacing/padding around the modal container edges (except for this detail on the close button icon). So my way of looking at these screenshot was most likely biased. Can you give me an example in the app where this need to modify the modal with |
Short description
In this PR, we are adding a new story for a basic confirmation modal, which is a very common use case for modals:
In addition, we are refactoring how we apply padding to the
Modal*components such that the inner components (ModalHeader,ModalBody, andModalFooter) are no longer responsible for knowing or declaring their own padding, as this can be more easily (and consistently) managed in theModalcomponent. Now, theModalcomponent applies its own outer padding and it uses the newishBox.gapprop to apply inter-child padding.PR Checklist
npm run validateand made sure no errors / warnings were shownCHANGELOG.mdpackage.jsonandpackage-lock.json(npm --no-git-tag-version version <major|minor|patch>) refVersioning