Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Security Solution][Resolver] Word-break long titles in related event… #75926
[Security Solution][Resolver] Word-break long titles in related event… #75926
Changes from 17 commits
b1e2bac
dd77617
889c2f2
69d3bf8
2ba7272
61a84a7
5c9b508
a96fb7b
f9c5278
0c80365
ff229ec
8389676
4c48fc5
2a10e03
b64164f
9efb55a
0be1d08
e7c1134
bccb7ab
9bc1de8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
ℹ️ moved to selector
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.
Referencing @kqualters-elastic comment from slack, I would cc a member of the EUI team to see about moving this logic there and getting that process started
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.
Yeah, I think that's a good idea for the long term, too. Seems like it would be useful in a lot of places. @chandlerprall has always been helpful, maybe he could give us an idea.
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.
for any context since I still have a terrible habit of saying too much there and not enough here: long term imo it might make sense to have this logic live in https://github.com/elastic/eui/tree/master/src/components/text as text_wrap.tsx and text_wrap.test.tsx, then used in the base text component conditionally based on the presence of a prop. Add a few snapshot tests that verify the presence of 's where you expect them, add as many test cases as needed
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.
Feel free to open an issue in EUI for discussion! It's possible we have this solved via a css utility class.
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.
ℹ️ pulled a bunch of this junk into a selector
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 see the rationale of having view specific logic like this tied with the component rather than the data. Even though the it doesn't actually change any of the raw data in state, I think of selectors as specifically handling some kind of expensive/calculative logic rather than anything display related. Maybe a bit off here, but I consider this more fitting in the
assets.tsx
file. I don't think that name is particularly accurate anymore, but the concept of:here are functions/components that, given some data, apply visual changes
is more representative. The name should be modified/changed when styles are cleaned up, but I think that may be a better compromise that will still allow this to be testable?
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.
Thinking out loud a bit, I think something a la a
style-utils
or something folder with separate file for visual components (cube assets etc...), text modifications, color modifications, and whatever else might make sense or not...probably something I missed here 😂 . Anyways, not really for this PR, but just thinking how we can begin to improve how we handle styles/visual changesThere 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 think most of this stuff here does things that Robert has referred to in the past as "business logic" like counting things, filtering things out, selecting which category to display, etc. (and prefers that they be done in selectors) but I completely agree about thinking a little more deeply about how we organize visual components. I ended up bringing the "break-hinting" stuff back up to the front, b/c @kqualters-elastic made a good point that putting it in the selector just creates another place it can break (and Robert already had that GeneratedText thing done) but that might be a good first entry in that
style-utils
(until it finds a home in EUI or something later).