Feature/rui modal #51
Conversation
|
Please see above draft PR for initial Modal core logic and Focus Trap implementation |
| --rui-modal__actions-margin: 10px 0; | ||
|
|
||
| /** | ||
| * @variable Actions section padding |
There was a problem hiding this comment.
Can we add alignment to the action box, and allow the user to configure these buttons to be aligned left/center/right
There was a problem hiding this comment.
I'll add styling hook for alignment :)
www/package.json
Outdated
| @@ -34,13 +34,21 @@ | |||
| "@mdx-js/mdx": "^1.0.0-rc.4", | |||
| "@mdx-js/react": "^1.0.0-rc.5", | |||
| "@rhythm-ui/button-react": "^1.0.0", | |||
| "@rhythm-ui/modal-react": "^1.0.0", | |||
There was a problem hiding this comment.
1 import is sufficient ;) (I think this is a generator issue as I've seen it elsewhere as well)
|
|
||
| .modal__heading > ::slotted([slot="heading"]) { | ||
| margin: 0; | ||
| font-size: 22px; |
There was a problem hiding this comment.
Why do we set fonts here instead of just relying on the users configuration for default text tags? Should we remove this? (or at least make it configurable)
| } | ||
|
|
||
| .modal__detail > ::slotted([slot="detail"]) { | ||
| font-size: 16px; |
There was a problem hiding this comment.
Same comment as the heading, should be set by core css for text tags
| * The size of the modal | ||
| */ | ||
| @property({type: String}) | ||
| public size: 'small' | 'medium' | 'fullscreen' = 'small'; |
There was a problem hiding this comment.
would prefer to use large over fullscreen here.
There was a problem hiding this comment.
@ricominten I think fullscreen makes sense here as it is styled to take up the full screen - so I don't agree with renaming it to large. Would adding a 3rd 'large' variant be okay instead?
| <div slot="detail"> | ||
| <p>Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis.</p> | ||
| </div> | ||
| <rui-button slot="confirm">Confirm</rui-button> |
There was a problem hiding this comment.
Is there a way to make a more generic action slot and just given them onClick events handlers? We have a current use-case where we have 3 action items in a modal and this solution would not support that.
| </rui-modal> | ||
| ``` | ||
|
|
||
| <rui-button onClick={() => { |
There was a problem hiding this comment.
Can we move these buttons inside the preview so that you can see how to open the modals?
|
@jnelssonsmith @jennasalau It might be worth looking into this for the behaviour of our focus trap: https://github.com/WICG/inert |
# Conflicts: # www/package.json # www/src/templates/Component/Component.tsx
Changes
@rhythm-ui/a11y-utilspackage that contains the newFocusTraphelper classFocusTrapclass, that creates and manages a focus trap@rhythm-ui/utilspackage that contains generate UUID function