Skip to content
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

Tidying CSS a bit and fixing IE and responsiveness #8

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

cchaos
Copy link

@cchaos cchaos commented Apr 6, 2020

I'll comment what I did in the diff, but essentially the flex and padding changes also fix mobile so you don't need special small screen styles.

All screenshots are IE, but look the same in Chrome

Without actions

Screen Shot 2020-04-06 at 16 45 37 PM

With actions

Screen Shot 2020-04-06 at 16 47 41 PM

Small screens

Screen Shot 2020-04-06 at 16 48 04 PM

Screen Shot 2020-04-06 at 16 48 18 PM


.euiCommentEvent__header {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mainly moved a lot of unnecessary nesting to ensure we don't create too much specificity.

}
}
.euiCommentEvent__header {
line-height: $euiLineHeight;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensures proper line-height when the items wrap

(https://github.com/philipwalton/flexbugs/issues/231#issuecomment-362790042)
*/
// sass-lint:disable-block mixins-before-declarations
@include internetExplorerOnly {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping the IE fix so it is only applied to IE browsers instead of all browsers. This means we drop support for IE, we can remove all of these.

&::after {
content: '';
min-height: inherit;
// Calculates the minimum height based on full header's min-height minus the vertical padding
min-height: $euiSizeXXL - $euiSizeS;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically the fix for IE creates a new pseudo element "child" flex-item and applies the minimum height to that child to force the height we want. But since it's nested within the __header which has top and bottom padding, if we don't subtract that padding, it increases the overall min-height.

Image 2020-04-06 at 5 00 22 PM

Sometimes it's good to try to understand what the CSS fix is doing so you know better how to tweak it to your implementation.

Comment on lines +53 to +54
// Push the actions far right
flex-grow: 1;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replaces what you were originally doing with justify-content: space-between to push the actions to the right. This fixes the the fix for IE 😆 ... Since when the fix was in it was creating a third child of the flex group, so it was putting space between the actions and the pseudo element, causing the actions to become more centered.

@andreadelrio andreadelrio merged commit 12eb136 into andreadelrio:comment Apr 6, 2020
@cchaos cchaos deleted the andreadelrio-comment branch April 9, 2020 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants