-
Notifications
You must be signed in to change notification settings - Fork 843
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
Another fixer PR #1188
Another fixer PR #1188
Conversation
- Added `maxWidth` to confirm modal prop docs - Allowing confirm modals to grow - Adding TS def for `maxWidth` - Fixing responsive widths
…led css - And moving animations to a utiltiy file so they only get compiled once
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. Code review. Minor comments.
@@ -40,20 +40,20 @@ export default class extends Component { | |||
<EuiFlexGroup justifyContent="spaceBetween"> | |||
<EuiFlexItem grow={false}> | |||
<EuiFlexGroup gutterSize="s"> | |||
<EuiFlexItem> | |||
<EuiFlexItem grow={false}> |
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.
How did we miss this when we looked at it. Nice.
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.
Because that's not how that should actually behave. IE sucks at nested flexes.
src/components/modal/_modal.scss
Outdated
@@ -30,8 +30,7 @@ | |||
} | |||
|
|||
.euiModal--confirmation { | |||
width: 450px; | |||
min-width: auto; | |||
min-width: 450px; |
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.
Dunno if it's worthwhile, but I used the euiFormMaxWidth
for this for the base. Do we need this one to be larger? Prolly ok to just remove the width setting alone?
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.
Definitely need the min-width, but sure I can change it to be the variable.
@@ -0,0 +1,42 @@ | |||
// Animations as utility so they don't get duplicated in compiled CSS |
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.
The hero we need.
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, code review
Background to readonly inputs: THANK YOU! |
🍾 |
@cchaos Does something need to happen with the docs site? Or maybe this is still pending a release... |
@w33ble Yeah it hasn't been released yet. It's just in Master. |
A couple of fixes in here
1. Add background to readOnly inputs
fixes #1129
2. Modal sizing fixes
maxWidth
toEuiConfirmModal
prop docsEuiConfirmModal
to growmaxWidth
3. Tag icon
fixes #1151
4. Fixed bottom bar layout in IE
fixes #983
5. Prevent comments from SASS invisibles files making their way into compiled css
Checklist
- [ ] This was checked against keyboard-only and screenreader scenarios