This task collects the suggested improvements after performing the design review of the Dialog component's MVP:
####Interaction
- Some of the properties of the enter and leave transition seem to not match the specifications. In the code, the enter and leave transitions share the same values. In the specs, timing function is different between the two. The designs also indicate a different duration for both transitions. To summarize:
| Transition | Value in the code ([[ https://gerrit.wikimedia.org/r/plugins/gitiles/design/codex/+/refs/heads/main/packages/codex/src/components-wip/dialog/Dialog.vue#455 | source) ]] | Value in Figma ([[ https://www.figma.com/file/8FOPeZwE4NKQsHnl2cRu29/Dialog---T284838?node-id=1451%3A13959 | source) ]]
| Enter/Fade-in transition | `transition-duration: @transition-duration-base` (=100ms); `transition-timing-function: @transition-timing-function-user` (=ease-out) | `transition-duration: @transition-duration-medium` (=250ms); `transition-timing-function: @transition-timing-function-system` (=ease)
| Leave/Fade-out transition | `transition-duration: @transition-duration-base` (=100ms); | `transition-duration: @transition-duration-medium` (=250ms);
- Loading state: Judging from [[ https://phabricator.wikimedia.org/T313773 | the MVP task ]], the loading state seemed to be in scope. But this doesn't appear to have been implemented.
####Visual
**Dividers**
In the MVP specs, there's a version of the Dialog that present dividers. The ticket also mentions that these should be applicable via a prop: "we can add a prop to add the dividers as props". Nevertheless, this doesn't seem to have been implemented yet.
**Overlay**
The value of the overlay's background-color should be `rgba( 255, 255, 255, 0.65 )`. The token `@background-color-overlay-light` is in the making ([[ https://gerrit.wikimedia.org/r/c/design/codex/+/834410 | see patch ]]) and should replace `@background-color-dialog-mask`. We should add a comment indicating that the value needs to be replaced by a token.
**Sizes** (generally)
From MVP task T313773:
- Small/Normal size: `@width-3200` (512px wide) for desktop (current size in normal dialogs in production) and full grid for mobile (leaving 16px on each side)
- Big size: 848px wide (we should add a `new width token`) for desktop (current size in big dialogs) and `fullscreen` for mobile
- Do we want to implement different props for `size` and `fullscreenOnMobile`?
**Width**
- Mobile min-width: In the code, there's a min-width applied to the dialog that matches its assumed mobile width (@min-width-dialog--mobile: 288px;). Following the designs, though, dialogs are full-width on mobile, so the `min-width` value of `.cdx-dialog` should be the token `@size-full`.
- Min-width: Starting at the tablet breakpoint, the min-width of Dialog (@min-width-dialog) should be 512px, as specified in the designs. This value will be replaced by the corresponding size token.
- Max-width: As specified in the description of the MVP ticket, the expected max-width of Dialog should be 848px (to be tokenized) instead of the current 700px.
**Height**
- Max-height: The value, `calc( @height-dialog-mask - ( @padding-dialog * 2 ) )` currently equals to 100vh-21.33px (the – to be corrected– Dialog padding). There doesn't seem to be clear specs regarding the top and bottom margins that would dictate this height. The MVP ticket only mentions "a max-height of 100vh minus some top and bottom "padding". We should probably use a value from our spacing scale here instead. 24px might be a good approximation? Pinging @bmartinezcalvo to confirm.
**Paddings**
`@padding-dialog: @size-base * ( 2 / 3 );` applies a homogenous 21.33px padding to the Dialog frame while, in the specs, sections have different paddings:
{F35541266}
**Margins**
We might want to reconsider how/where the current margins are applied. In the version with dividers, the margin of the Dialog's body would be 12px instead of 8px (which would correspond to the bottom margin of the header).
{F35541322}
(The body would also have an additional bottom margin of 16px, but that could be added separately without requiring any further changes.)
**Fonts**
Following the designs, the dialog title should be 16px (1em), with a 20px line-height (1.25). The body, 14px (0.875em) with a 20px line-height (1.4285714286*).
In the code, the same font size was applied to both title and body text (1em = 16px). This means that, under Vector, both elements will probably be sized 14px. This is not so much a question about avoiding the compounding effect, but about how to maintain the hierarchy applied in the design in production. A solution would be to increase the size of the title to 18px (1.125em with a 1.25 line-height, Heading S style in Figma). Pinging @bmartinezcalvo so she can consider this.
(*Just for visibility: This line-height will after all most likely not be introduced into the system, but will be replaced by 1.6.)
####Token implementation
We should avoid defining local variables as much as possible and, instead, use the available base/decision tokens directly as values. Whenever the necessary tokens are missing, we should add comments indicating that the temporary discrete value will need to be replaced by a token as soon as it's available. We also should never use theme tokens as values (e.g. `@padding-dialog-mask: @size-absolute-100;`), or tokens with a different intent (e.g. `@margin-dialog-body: @size-base * 0.25;`). These suggestions would translate as follows in the current version of the code:
```
// TODO: Tokenize. [Applies to everything that remains under this line]
@background-color-dialog-mask: rgba( 255, 255, 255, 0.65 ); [Value updated, see section Visual>Width. Once the token is available, this variable should be deleted]
~~@min-height-dialog-mask: @size-full;~~ [not needed, use @size-full directly as min-height value of .cdx-dialog-mask]
@height-dialog-mask: 100vh; [Once the size token is available, this variable should be deleted]
@width-dialog-mask: 100vw; [Once the size token is available, this variable should be deleted]
~~@min-width-dialog--mobile: 288px;~~ [Should be removed and replaced by `@size-full`, see Visual>Width]
@min-width-dialog: 512px; [Value updated, see section Visual>Width. Once the token is available, this variable should be deleted]
@max-width-dialog: 848px; [Value updated, see section Visual>Width. Once the token is available, this variable should be deleted]
@max-height-dialog: calc( @height-dialog-mask - ( @padding-dialog * 2 ) ); [Value should be replaced based on design decision, see Visual > Height. Calc should involve tokens and be used directly as a value. Once the tokens are available, this variable should be deleted]
@padding-dialog: @size-base * ( 2 / 3 ); [Value should be updated/split, see Visual > Paddings. To be removed once values can be replaced by tokens]
@padding-dialog-mask: 16px; [Should be replaced by a base spacing token when available]
@margin-dialog-body: 8px; [The variable should be replaced by a base spacing token when available]
@margin-dialog-footer: 16px; [The variable should be replaced by a base spacing token when available]
@margin-dialog-actions: 8px; [The variable should be replaced by a base spacing token when available]
```
####Demo
- I'm curious to hear what @bmartinezcalvo will think, but I'd say that including the props/configuration table inside the body of the [[ https://doc.wikimedia.org/codex/main/components/demos/dialog.html#configurable-dialog | configurable ]] (simple) Dialog is confusing. I think that the workflow would be good enough if the table were included in the demo page (under the 'Open dialog' button). Users could change the config and then open the Dialog to check the result. If this is not acceptable, then we could provide the config in a side panel to the right of the open Dialog? But that sounds more costly.
=== Acceptance Criteria for Done
**Implement interaction amendments**
[x] correct transition values
[] loading state
**Implement visual amendments**
[x] Dividers addable via props
[] Overlay mask
[x] Design needs to provide token
[] Unify on name (overlay vs mask)
[] Sizes
[x] Add normal size `@width-3200` dialog
[x] Add big size (=848px) dialog
[] Add full size dialog
[] Add props for
[x] `size`
[] `fullscreenOnMobile`
[] Width
[x] Mobile `min-width` should be `@size-full` on mobile instead of `@min-width-dialog--mobile: 288px;`
[x] Tablet breakpoint `min-width` should be 512px, as specified in the designs. `min-width-dialog` carrying corresponding size token.
[x] `max-width` should be `848px` (to be tokenized) instead of the current `700px`.
[] Design will provide new size token
[x] Height
[x] `max-height`: The padding value in `calc( @height-dialog-mask - ( @padding-dialog * 2 ) )` should be updated to `40px`.
[x] Padding should be different, not homogenous `@padding-dialog: @size-base * ( 2 / 3 )` equivs `21.33px`
[x] Margin needs increase at dialog content to `12px`
[] Font:
[] dialog title needs to carry 16px (`1em`) `font-size`, with a 20px `line-height` (`1.25`).
[] dialog body needs to carry, 14px (`0.875em`) with a 20px `line-height` (`1.4285714286*`).
[] Token implementation
[] Replace theme tokens with base or component tokens
[] Replace calculated spacing tokens with design decision tokens (flag if needed)
[] Demo?
[] Demo props table in simple dialog
[] Replace “close” with “cancel”