refactor: Rework modal padding

[pawelgrimm]

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, and ModalFooter) are no longer responsible for knowing or declaring their own padding, as this can be more easily (and consistently) managed in the Modal component. Now, the Modal component applies its own outer padding and it uses the newish Box.gap prop to apply inter-child padding.

PR Checklist

• [x] Updated docs (storybooks, readme)
• [ ] Executed npm run validate and made sure no errors / warnings were shown
• [ ] Described changes in CHANGELOG.md
• [ ] Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
• [ ] Reviewed and approved Chromatic visual regression tests in CI

Versioning

[pawelgrimm]

Ah, this change prevents dividers from taking up the whole modal width 🤦

[gnapse]

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 ModalHeader and ModalBody in it, each with its padding, but we do not want a padding to the entire modal due to the custom sidebar.

I can also mention this comment https://github.com/Doist/reactist/pull/789#issuecomment-1621762231.

---

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?

[pawelgrimm]

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?

I was just flipping through Storybook and noticed that some of the examples looked off.

---

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?

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 exceptionallySetClassName. For example, check out our delete-section-confirmation-modal vs the example in my OP vs the Figma mockup:

Todoist	This PR	Figma

[gnapse]

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 `exceptionallySetClassName`. For example, check out our delete-section-confirmation-modal vs the example in my OP vs the [Figma mockup](https://www.figma.com/file/LYlWNzvhMDh907l07mPPQk/Product-Library---Web?type=design&node-id=1802-345&mode=design&t=vdZdac3HZEcKprmh-4):

Todoist	This PR	Figma

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).

[pawelgrimm]

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 Modal primitives without any additional modification (including exceptionallySetClassName. Sound good?

[gnapse]

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?

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 exceptionallySetClassName has been necessary and why? Happy to move the discussion to some internal medium if it's starts to not be entirely about Reactist, so we keep this discussion public.