-
Notifications
You must be signed in to change notification settings - Fork 831
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
[EuiText] Updated euiTextContrainedMaxWidth
Value
#6146
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
**Bug fixes** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [starting a new thread to not make the old one go a million comments long] Aghh I hate to 180 on this so much but it looks like there actually is a very weird / buggy interaction with To contrast, here's the before/after using a static container:
The widths of the first example in both links are exactly the same (using 75% correctly). So I went into
And here are the results with And here are the results with So... basically, it is a bug, but I think it's a bug specifically with EuiPopover and CSS min/max functions. I'm relatively certain it's EuiPopover's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there are a couple approaches/fixes to this issue:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After a little more testing, it's actually not
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, this doesn't fix Maps' use case, as they likely still do not want a 75% wide popover, so again they should be advised to stop using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To recap:
Sound right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Need to remember to account for the container config also: https://elastic.github.io/eui/#/layout/popover#constraining-a-popover-inside-a-container There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After poking a little more into the guts of So, as Chandler mentioned in the above thread, we may need to start modifying EuiPopover to manually set width/height styles where we're currently not... but that may also cause other unexpected shenanigans with popover down the road. Fun times! |
||
|
||
- Updated `euiTextContrainedMaxWidth` to ensure text content fits doesn't extend the bounds of the screen |
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.
Just curious, if we keep this as
max(64ch, 75%)
but usemax-width
what are the results?I'd be ok with a slight update to width as long as the constraint is reasonable
Original request for the change:
#5895 (comment)
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.
Ok so it looks like
max-inline-size
is not the problem. It's the75%
param, which is much too wide.Seems like we could keep
logicalCSS('max-width')
and updateeuiTextConstrainedMaxWidth
to64ch
.@constancecchen Any insight here from making this change initially?
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.
Thanks for digging further @thompsongl ! Looking at the
.euiText--constrainedWidth
css,it looks like the styles were incorrectly combined to
max(64ch, 75%)
.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.
To be honest Caroline made the change/request within my PR, I didn't think too much about it 😅 The comment you linked (+ 32690fa) is the only context I had.
++ to
max-inline-size
not being the issue++ to Chandler's discovery, it looks like what we actually want is
min(64ch, 75%);
instead ofmax()
?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.
I like the
min
idea but it still doesn't seem to work. The popover is still huge in the Maps example.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.
Very interesting. It "worked" in Kibana because it was "broken" in EUI. Then as a standard, consumers who are have large bits of content would be responsible for the sizing of the popover via 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.
I'll reach out to them in the Slack thread and use this convo as a reference
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.
+1 to the suggestion for Kibana to update. We cannot predict what text / flow is going to be present in a popover, and solving for the maps set up will likely cascade a
failurechange to another location, etc. This amount of text in a popover already makes assumptions on screen size, etc and has usability concerns that, IMO, only the app can properly address.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.
Not to contradict myself (but I am anyway), one thing we should probably consider is modifying popover to have a max constraint on both dimensions to ensure it fits in the screen.
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.
@chandlerprall see below thread, our
popover_positioning
service is having issues correctly calculating the content whenpercentages are being used - normally our popover should be correctly modifying its dimensions to ensure it fits on the screen 🤔max()
/min()
CSS functions