-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
feat: css template add/edit modal #11296
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11296 +/- ##
==========================================
- Coverage 65.69% 65.68% -0.01%
==========================================
Files 835 836 +1
Lines 39659 39771 +112
Branches 3610 3647 +37
==========================================
+ Hits 26052 26122 +70
- Misses 13498 13540 +42
Partials 109 109
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ee66ef2
to
8518729
Compare
onHide={hide} | ||
primaryButtonName={isEditMode ? t('Save') : t('Add')} | ||
show={show} | ||
width="800px" |
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.
FWIW, we can do some fun stuff with this, e.g. width={"calc(100% - 100px)"}
if you ever feel like it :D
If/when there are a lot of these modals kicking around, we should probably add a limited number of preferred values to the Theme or a constants file.
<span className="required">*</span> | ||
</div> | ||
<StyledCssEditor | ||
height="240px" |
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.
Might not be worth the battle, but I'm curious if we could make the modal take up as much room on the screen as it can, and use a flexbox layout within the modal, which would let THIS component take up as much of the layout as possible, rather than limiting its height with a static value.
b16f8e1
to
3fdcf10
Compare
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.
lgtm!
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://www.loom.com/share/8d206d73074d4c5bbf804d2e60924b29
TEST PLAN
ADDITIONAL INFORMATION