Conversation
rwood-moz
left a comment
rwood-moz
left a comment
There was a problem hiding this comment.
Tried it out locally, ran the updated component tests (all pass), and had a look at the ModalDialog in the updated storybook. LGTM!
* The designs show a services specific logo on top (Thundermail), which isn't part of this change, since it can just be inserted into the header slot together with the title. Please let me know if we want a dedicated slot or story for that.
IMO a dedicated slot isn't necessary if the header can just be used, but I'll leave that up to you and @davinotdavid whatever you think. If you leave it in the header maybe you could add to the storybook an example with a header with the Thundermail logo in it if you think that would be useful.
davinotdavid
left a comment
davinotdavid
left a comment
There was a problem hiding this comment.
Thanks for the work so far! Looks pretty good. A few things only to adjust:
- There's also a variant in Zeplin that has a product logo on the top left. Perhaps we should add a slot for that just in case?
| <div v-if="$slots.notification" class="modal-notification"> | ||
| <slot name="notification"></slot> | ||
| </div> |
There was a problem hiding this comment.
This is interesting and I like the idea of having a dedicated space for the notification. However, doing it like so as a sibling of the modal-body and having the modal-body be the scrollable point, it may cause some cropping:
There was a problem hiding this comment.
Good find! I moved the notification slot inside the .modal-body class now. My intention here was to have the notification always visible, even when scrolled down.
There was a problem hiding this comment.
Having a padding-top to control the spacing here makes it a bit harder to adjust the layout overall IMHO.
The spacing around the close button seems different than Zeplin and if we were to measure the title to the top it would be 6rem instead of 4rem (although I'd argue that your positioning of the title is better):
There was a problem hiding this comment.
@davinotdavid I removed the padding and tried to do the spacing element-wise, taking all possible slot configurations into account. I find the layout of this component difficult in general, also Zeplin and Figma seem to differ a lot in that regard. Maybe we can adjust this in a follow-up PR after a desgin-review.
There was a problem hiding this comment.
Yeah it is indeed tricky because of the absolute positioned close button there I believe and designs are not really aligned either in Zeplin so agreed that it can be fixed in a future PR.
However, for modals without title, I believe we should still add some padding at the top otherwise it looks a bit strange:
With actions
Simple
There was a problem hiding this comment.
Yes, agreed. I added 3rem top margin for the body if it's standalone now. Thanks for this suggestion!
|
@davinotdavid Thanks so much for your review! I addressed all of your points. Since you requested it explicitly (see earlier discussion with Rob), I added a slot 
|
davinotdavid
left a comment
davinotdavid
left a comment
There was a problem hiding this comment.
LGTM after the reviews have been addressed, thanks!
One final thing:
3 participants
What changed?
Added a new optional notification slot to ModalDialog, rendered between the header and body and only shown when content is provided. Extended ModalDialog.test.js to cover the slot's presence and absence, and added a "With Notification" story demonstrating a NoticeBar info message.
This PR also aligns the styles of the ModalDialog component with the current design.
Why?
Error or info messages shown inside a ModalDialog currently have no dedicated place and end up hand-placed in the body slot. Now they have a defined place.
Limitations and Notes
Applicable Issues
Closes #234
Closes #321
QA Log
vitest run test/components/ModalDialog.test.js, all 16 tests pass.vue-tsc --noEmitandeslinton the changed files, no errors.WithNotificationStorybook story and compared to the design.Screenshots
Style changes:
With notification story
