[CCUBE-2221][ZZ] feat: migrate ESignature to Modal v2 - #1428
Conversation
5aadf13 to
1241d04
Compare
039d232 to
5e2f69a
Compare
| await act(async () => { | ||
| jest.advanceTimersByTime(500); | ||
| }); | ||
|
|
There was a problem hiding this comment.
wondering why the assertion on signature modal was removed, and we need to manually handle the timers here?
There was a problem hiding this comment.
the assertion was to check whether or not it's visible, but we're mount/unmounting for modal v2
modal v2 has a transition animation with 300ms, so that's just to skip the animation
There was a problem hiding this comment.
why is it not needed for the other tests?
| column-gap: 0; | ||
| } | ||
|
|
||
| && > button { |
There was a problem hiding this comment.
should we reduce the specificity of the footer button styles?
| margin: auto; | ||
| padding: ${Spacing["layout-xxl"]} ${Spacing["layout-sm"]}; | ||
| ${MediaQuery.MaxWidth.sm} { | ||
| &[data-mobile-landscape][data-mobile-landscape] { |
There was a problem hiding this comment.
why is the data-attribute specified twice?
There was a problem hiding this comment.
to increase the specificity because we're overriding what's inside the modal card
5e2f69a to
a6832f9
Compare
qroll
added
type: chore
31d0194 to
dae2cc2
Compare
| max-height: none; | ||
| margin: 0 auto; | ||
| padding: ${Spacing["spacing-16"]}; | ||
| box-shadow: ${Shadow["lg-strong"]}; |
There was a problem hiding this comment.
what does the box shadow do?
There was a problem hiding this comment.
Modal.Box had this styling before but ModalV2.Card has xs-strong. Just making sure the styling is similar as the old one as much as possbile
| export const scrollableModal = css` | ||
| /* increase specificity as the styles are overwritten */ | ||
| export const modalCard = css` | ||
| && { |
There was a problem hiding this comment.
do we still need the increased specificity?
There was a problem hiding this comment.
yes still needed here to override the ModalV2.Card styles
| box-shadow: ${Shadow["lg-strong"]}; | ||
| } | ||
|
|
||
| & > [data-has-close-button] { |
There was a problem hiding this comment.
is there another way we can style the content without using this data-has-close-button selector? as that's an internal implementation for the modal. it also feels like this could be something that ModalV2 is supposed to handle properly with the fullscreen prop
There was a problem hiding this comment.
will find a workaround then
| width: 100%; | ||
| margin: auto; | ||
| padding: ${Spacing["layout-xxl"]} ${Spacing["layout-sm"]}; | ||
| export const modalHeader = css` |
There was a problem hiding this comment.
ideally with the new ModalV2.Header we no longer need to specify so many custom styles. what are the changes here and why are they needed?
There was a problem hiding this comment.
this was because the header fonts and spacing somehow after changing to using ModalV2 is way different to what we were using when it's still Modal
| export const modalContent = css` | ||
| && { |
There was a problem hiding this comment.
ideally we should avoid using the && selector
There was a problem hiding this comment.
can find work arounds for this then. will comment back if no solution is found
| ${MediaQuery.MaxWidth.sm} { | ||
| &[data-mobile-landscape] { |
There was a problem hiding this comment.
hmm why are these selectors nested?
There was a problem hiding this comment.
to increase specificity as well
…ard on mobile specific styling
3 participants
Type of changes
Description of changes
Checklist