-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: preview modal ui to match figma #3798
base: MI-521
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Some comments about using "Important"
@@ -67,7 +68,7 @@ function PostModerationModal({ | |||
size={Modal.Size.Large} | |||
kind={Modal.Kind.FlexibleTop} | |||
> | |||
<Modal.Body className="gap-6"> | |||
<Modal.Body className="gap-6 !p-6"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not override the padding without using important? I think we should avoid it whenever necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this is not possible due to the order in which the classes are written by TailwindCSS.
We have discussed this issue internally in the past and proposed using twMerge but it has not been adopted yet :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this it's fine, ideally all modals follow guidelines, but this is a weird modal I guess.
)} | ||
<ModalClose onClick={modalProps.onRequestClose} /> | ||
<ModalClose | ||
className="relative !right-0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModalClose actually accepts props to adjust right- :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Didn't see before, thanks for suggesting! 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use props on the close one
)} | ||
<ModalClose onClick={modalProps.onRequestClose} /> | ||
<ModalClose | ||
className="relative !right-0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModalClose actually accepts props to adjust right- :)
@@ -67,7 +68,7 @@ function PostModerationModal({ | |||
size={Modal.Size.Large} | |||
kind={Modal.Kind.FlexibleTop} | |||
> | |||
<Modal.Body className="gap-6"> | |||
<Modal.Body className="gap-6 !p-6"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this it's fine, ideally all modals follow guidelines, but this is a weird modal I guess.
…-modal-preview-ui
Changes
Events
Did you introduce any new tracking events?
Experiment
Did you introduce any new experiments?
Manual Testing
Caution
Please make sure existing components are not breaking/affected by this PR
Preview domain
https://fix-modal-preview-ui.preview.app.daily.dev