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
[UI Framework] [K7] Add unstyled KuiModal components. #13199
[UI Framework] [K7] Add unstyled KuiModal components. #13199
Conversation
cjcenizal
commented
Jul 28, 2017
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. I can cleanup the styling, but left some comments there just for stuff I saw offhand. If you have a chance you might want to add some JS to effect the body in mobile as mentioned in one of the comments. Can do in a later PR of course.
justify-content: flex-end; | ||
|
||
> * + * { | ||
margin-left: 5px; |
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.
Ditch the pixels. If this is for button spacing use $kuiSize
display: flex; | ||
justify-content: space-between; | ||
align-items: center; | ||
padding: $kuiSize; |
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.
Saw "unstyled" so please ignore this if you just want me to clean it up, which I'm happy to do and probably what you expected. The rest of this is just me explaining some design stuff assuming you're curious. Just paying it forward after all the react help.
Use uniform space between objects.
Right now you're padding the header, then padding the body, then padding the footer. This causes your spacing to be...
16px
header
16px
16px <-- this will look off
body
16px
16px <-- this will look off
footer
16px
That's not to say that you always use uniform spacing in things, it's just something you generally do in block level layout elements (with the assumption that the stuff inside (like a super large title that needs space) adds the unique flavor). You want it to be the same space between items vertically as you're spacing them horizontally. What if the header doesn't exist? Assume that each item is optional in the layout.
You want $kuiSizeXL
(32px) as your base here. That way the modal layouts match the page layout spacing and it "feels" connected. You can also do similar things by min-sizing some of these.
Don't forget responsive stuff! Usual way to handle modals in responsive layouts is to force the body to max-height: 100vh
, then forcing the actual modal to be 100% h/w with an overflow-y: auto
. That way you only "browse" against the modal and not the underlying content. I mention this one because you'll likely need to do some JS for this, and it's not something I could do.
e4079af
to
275cb06
Compare
Thanks for walking me through your approach! |