Add Modal component guideline by Cuervino · Pull Request #1033 · scality/core-ui

Cuervino · 2026-03-16T20:19:28Z

Description

This PR introduces the first complete design guideline for the Modal component in Storybook, along with a rewrite of the existing stories to match the documented
conventions.

The modal.mdx file that previously existed was a placeholder (a single Canvas and two story references with no documentation). It has been replaced by
modal.guideline.mdx, a full guideline covering all usage decisions for the component.

The modal.stories.tsx file has been rewritten: the existing stories used "Yes" / "No" as button labels, which violates the modal-button-forbidden-label valalint
rule. Footer alignment was also incorrect (space-between instead of flex-end). A new DestructiveModal story has been added to illustrate the alertdialog pattern.

Change list

Change	File	Comment
Added	`stories/modal.guideline.mdx`	Full design guideline for the Modal component
Deleted	`stories/modal.mdx`	Was a placeholder, replaced by the guideline
Rewritten	`stories/modal.stories.tsx`	Correct button labels, footer alignment, new DestructiveModal story
Updated	`.storybook/preview.js`	storySort updated to display Guideline before stories for all components

Design

Guideline covers:

Role — dialog vs alertdialog: when to use each, ESC behavior, close button presence
Size — min-width 480px, max-width 90vw
Scroll — avoid when possible, vertical padding against viewport, visible scrollbar on overflow
Title conventions — mirrors the trigger action verb; question mark for destructive/irreversible actions
Footer — always required, right-aligned, 2 buttons standard / 3 acceptable, no links
Destructive modals — alertdialog role, danger variant, resource name mandatory in body if it exists
Async — isLoading under 3s, contextual message over 3s, no modal for long operations
Forbidden patterns — no modal on modal, no event-triggered modals at app level

Breaking Changes

No

- Add stories/modal.guideline.mdx with full usage documentation: roles, close behavior, size rules, footer constraints, destructive modal patterns, async handling, and forbidden patterns - Rewrite stories/modal.stories.tsx with correct button labels, proper footer alignment (flex-end), and a DestructiveModal story - Update .storybook/preview.js storySort to display Guideline before stories for all components

bert-e · 2026-03-16T20:19:33Z

Hello cuervino,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2026-03-16T20:19:39Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

JBWatenbergScality · 2026-03-17T09:29:51Z

+- `title` is always required and always text. It mirrors the verb of the action that triggered the modal: the button "Delete node" opens a modal titled "Delete node?".
+- For standard modals, the title describes the action: "Edit settings", "Create bucket".
+- For destructive or irreversible actions, the title is phrased as a question: "Delete node?", "Revoke access?". The question mark signals that a decision is required.
+- `subTitle` is used for step indicators in multi-step modals only (e.g. `"Step 1/2"`).


This is opposed to:

❌ Do not use a modal for:

Multi-step flows — use a full page or wizard instead

Are we missing some use case where we allow multi-steps modals ?

I believe the subtitle attribute is shared by both the modal and the full-page form component.
However, I suspect the modal header might not actually use it for descriptive text (at least not in the standard sense). It’s possible it’s being only used for the '* indicates required fields' disclaimer.

I'll remove this mention to the multi-step modal. The Download modal is the only kind of multistep modal I can think of, but we don't display steps per say.

Done — I've removed the multi-step reference from subTitle. The description now reflects its actual role: it occupies the header slot when no close button is present, and can be used for contextual information such as a step indicator or a required-fields disclaimer.

JBWatenbergScality · 2026-03-17T09:31:08Z

+## Role: `dialog` vs `alertdialog`
+
+The `role` prop controls how assistive technologies treat the modal and defines the dismissal behavior.
+
+| `role` | When to use | ESC closes it | Close button |
+|---|---|---|---|
+| `dialog` (default) | Standard interaction — form, optional confirmation | Yes | Present |
+| `alertdialog` | Critical or destructive action requiring a forced decision | No | Omit |
+
+Use `alertdialog` when accidentally dismissing the modal would be dangerous or meaningless (e.g. a delete confirmation — ESC should not silently cancel the user's intent).
+
+## Close button and backdrop
+
+- The **close button** (×) in the header is present by default via the `close` prop. Omit it only when using `role="alertdialog"` — the user must explicitly choose an action.
+- **Clicking the backdrop never closes the modal.** This prevents accidental data loss, especially in forms.
+- **ESC** closes `dialog`-role modals. It does not close `alertdialog`-role modals.


I'm unsure about those esc key rules. I'm afraid it can restricts accessibility.

Do you mean you are pro or against it?

I think I get the concern, but I don't think it's actually a trap -- WCAG 2.1.2 doesn't require ESC specifically, just that keyboard users can get out. And they can, via the footer buttons. The whole point of blocking ESC on an alertdialog is to force an explicit choice, not to trap anyone. Is that what you were talking about?

JBWatenbergScality · 2026-03-17T09:33:43Z

+## Forbidden patterns
+
+- **No modal on top of another modal.** If a flow requires a second modal, redesign as a full page or wizard.
+- **No event-triggered modals** at the application level (login, WebSocket events, etc.) — race conditions between modals create an uncontrollable stacking experience.


We have the ISV exception case.

Yeah, indeed, I'll say that we need to limit it, not to forbid it.

Good point, I changed 'No' to 'Avoid' -- less absolute, leaves room for edge cases without making them a recommendation in the guideline.

bert-e · 2026-03-17T16:17:58Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

…tion

bert-e · 2026-03-18T15:03:28Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

bert-e · 2026-03-20T16:25:16Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

Cuervino · 2026-03-20T16:27:09Z

/approve

bert-e · 2026-03-20T16:27:21Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/1.0

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2026-03-20T16:28:19Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/1.0

Please check the status of the associated issue None.

Goodbye cuervino.

JBWatenbergScality reviewed Mar 17, 2026

View reviewed changes

MonPote reviewed Mar 17, 2026

View reviewed changes

Comment thread stories/modal.guideline.mdx

Cuervino added 3 commits March 17, 2026 18:44

docs: align ESC behavior description with actual component implementa…

4b785e5

…tion

docs: broaden subTitle description, remove multi-step modal reference

8a9e1ae

docs: soften event-triggered modals rule from forbidden to discouraged

9926754

Cuervino requested review from JBWatenbergScality and MonPote March 17, 2026 18:11

MonPote approved these changes Mar 18, 2026

View reviewed changes

JBWatenbergScality approved these changes Mar 20, 2026

View reviewed changes

bert-e merged commit 9e03cd1 into development/1.0 Mar 20, 2026
4 checks passed

Conversation

Cuervino commented Mar 16, 2026

Description

Change list

Design

Breaking Changes

Uh oh!

bert-e commented Mar 16, 2026

Hello cuervino,

Uh oh!

bert-e commented Mar 16, 2026

Waiting for approval

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bert-e commented Mar 17, 2026

Waiting for approval

Uh oh!

bert-e commented Mar 18, 2026

Waiting for approval

Uh oh!

bert-e commented Mar 20, 2026

Waiting for approval

Uh oh!

Cuervino commented Mar 20, 2026

Uh oh!

bert-e commented Mar 20, 2026

In the queue

Uh oh!

bert-e commented Mar 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants