[EuiCommentEvent] Restore child classNames removed in Emotion conversion#6089
Merged
cee-chen merged 7 commits intoelastic:mainfrom Aug 1, 2022
Merged
[EuiCommentEvent] Restore child classNames removed in Emotion conversion#6089cee-chen merged 7 commits intoelastic:mainfrom
cee-chen merged 7 commits intoelastic:mainfrom
Conversation
- currently used in Kibana for CSS hooks/test selectors
- in favor of just calling headerStyles children directly (since there are no concatenated styles)
…ng a separate const - I'm not seeing a super strong reason for the separate const personally
Contributor
Author
|
I need to duck out for a few hours for an appointment but once I'm back I will be updating the migration guidelines (#5685 (comment)) with these recommendations! |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6089/ |
elizabetdev
approved these changes
Jul 28, 2022
Contributor
elizabetdev
left a comment
elizabetdev
left a comment
There was a problem hiding this comment.
Thanks, @constancecchen. LGTM! 🎉
chandlerprall
approved these changes
Jul 29, 2022
cee-chen
pushed a commit
that referenced
this pull request
Aug 2, 2022
…ion (#6089) * Restore `__` child classNames to EuiCommentEvent - currently used in Kibana for CSS hooks/test selectors * Add `data` attribute to replace old modifier classes * [cleanup] Remove emotion CSS with no styles, move modifiers above children * [cleanup] Remove unnecessary const declarations - in favor of just calling headerStyles children directly (since there are no concatenated styles) * [opinionated] move eventHeader into final JSX directly instead of being a separate const - I'm not seeing a super strong reason for the separate const personally * Update EuiComment snapshots * Changelog
This was referenced Aug 2, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Summary
Several Kibana tests and CSS overrides were targeting these classNames: elastic/kibana@1bdae6b
After some team discussion on Slack, we've decided that for the Emotion conversion, we're generally opting to not remove
__childclassNames (especially on DOM elements that aren't customizable via consumers/props). They serve as useful hooks/markers for consumers to use.Modifier
--classNames, especially maps, are fair game to be removed especially if replaced by adata-attribute (I've added one here for event types), but should be checked in Kibana first for usage.Notes
While I was here I did some minor code cleanup. The first 2 commits (+ snapshots) are the only required changes
Checklist