[SharedUX] Replace Sass with Emotion, Round 1 #199885

tsullivan · 2024-11-12T22:47:29Z

Summary

Part of https://github.com/elastic/kibana-team/issues/1082

Selects certain Sass files to replace with styles declared with Emotion. This PR does not include any changes that would be noticeable by end-users. It changes the internals to use a different technology for styling components.

~~Some className attributes have been kept, because they are referenced in JS and tests.~~ Update: all classNames that are no longer needed for styling purposes have been removed.

If the className was needed for tests, it has been replaced with a test-subj.
If the className was used as a selector in production code, it has been replaced with alternative JS.

References

tsullivan · 2024-11-12T22:51:00Z

packages/core/chrome/core-chrome-browser-internal/src/ui/loading_indicator.scss

-.euiHeaderSectionItem .euiButtonEmpty__text {
-  // stop global header buttons from jumping during loading state
-  display: flex;


I did not port these styles over, because I do not see the global header buttons jumping around during loading state.

I just added this in a recent PR. See if you notice it when throttling your browser... in particular, the spaces button (loading rectangle) would jump up and right in a flash.

Checked locally, and it looks like the jump is there if this is excluded.

CleanShot.2024-11-13.at.10.03.40.mp4

Thanks @ryankeairns! I restored these styles in 8e4cd1d. I will try to improve my test method

…ing state

tsullivan · 2024-11-13T18:45:48Z

...es/core/chrome/core-chrome-browser-internal/src/ui/header/__snapshots__/header.test.tsx.snap

@@ -71,6 +72,7 @@ Array [
                aria-hidden="true"
                aria-labelledby="elasticMark"
                class="chrHeaderLogo__mark"
+                css="You have tried to stringify object returned from \`css\` function. It isn't supposed to be used directly (e.g. as value of the \`className\` prop), but rather handed to emotion so it can handle it (e.g. as value of \`css\` prop)."


As far as I can tell, it is normal to see this message in a snapshot file when the component uses a css prop that gets an object returned from the css function.

eokoneyo · 2024-11-20T12:31:36Z

packages/core/chrome/core-chrome-browser-internal/src/ui/header/header_logo.tsx

@@ -102,11 +119,12 @@ export function HeaderLogo({ href, navigateToApp, loadingCount$, ...observables
        <img
          src={customizedLogo}
          className="chrHeaderLogo__mark"


I know we left these className in here because it's targeted some test files, what do you think about swapping this for testIds especially that it wont be apparent to some other person much later?

@eokoneyo thanks for taking a look at this. I would prefer to leave this as it is. It will help this PR, and others like it, to avoid ballooning the refactoring into something larger than it needs to be. Changing this code could impact other code owners as well, and then add the need for more teams to review.

Edit: let me think more about this. One of the class names is used as a selector for production code. That probably isn't the best practice and there should be a more acceptable way.

bb45851 replaced a className with a testId, since it was only needed for a test

Heyy @tsullivan , @eokoneyo ,

Just curious from investigations perspective, I agree that tests should target test-ids but is there a downside of keeping classNames here?

@logeekal The downside of using classNames for non-style purposes is that classNames are not a public contract, so they shouldn't be used for other things.

As we're replacing CSS/SASS with Emotion, for thoroughness, the typical followup will be to remove the className from the DOM, for thoroughness. We could break integrations if we fail to realize that some place in the repo is using that className as a selector in production code.

Relevant: 24f6977

tsullivan · 2024-11-21T23:22:27Z

x-pack/plugins/security_solution/public/timelines/components/timeline/helpers.tsx

@@ -184,7 +184,7 @@ export const focusUtilityBarAction = (containerElement: HTMLElement | null) => {
 * Resets keyboard focus on the page
 */
 export const resetKeyboardFocus = () => {
-  document.querySelector<HTMLAnchorElement>('header.headerGlobalNav a.chrHeaderLogo')?.focus();


@elastic/security-threat-hunting-investigations in reviewing this, let's consider accessibility. The way this works should keep a logical and intuitive flow in keyboard navigation.

@michaelolo24 / @kqualters-elastic Any historical context here? Was this just added to remove keyboard focus?

As suggested by @tsullivan in the slack thread , I think I agree that we can use document.body.focus() as well. Do you have any opinion?

I am not sure on the context of this to be honest, seems it's used in conjunction with https://github.com/elastic/kibana/blob/main/x-pack/plugins/timelines/common/utils/accessibility/helpers.ts#L772 . What exactly is wrong with this? "The way this works should keep a logical and intuitive flow in keyboard navigation." is this not the case now?

What exactly is wrong with this? "The way this works should keep a logical and intuitive flow in keyboard navigation." is this not the case now?

@kqualters-elastic this is what SharedUX team needs the @elastic/security-threat-hunting team to test and verify, because we don't know how to access all of the parts that this change could affect. The context is that this code needs to change because .chrHeaderLogo is going away for cleanup purposes.

Let us know if you or a delegate can confirm that the functionality is not negatively impacted, or wish to see this modified in some way. Thanks!

tsullivan · 2024-11-21T23:23:15Z

@eokoneyo fully removing the class names had minimal effects. Can you re-review?

elasticmachine · 2024-11-22T01:22:42Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 24f6977

Failed CI Steps

x-pack/test/alerting_api_integration/security_and_spaces/group1/config.ts

Test Failures

[job] [logs] x-pack/test/alerting_api_integration/security_and_spaces/group1/config.ts / alerting api integration security and spaces enabled Alerts - Group 1 alerts backfill rule runs ad hoc backfill task should run all execution sets of a scheduled backfill and correctly generate alerts

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	448	434	-14

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	13.4MB	13.4MB	-83.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	454.2KB	449.8KB	-4.4KB

History

💔 Build #252944 failed bb45851
💚 Build #252025 succeeded 1229f05
💔 Build #251574 failed 0f6006c
💔 Build #250991 failed 8e4cd1d
💔 Build #250639 failed 3ce08ce

clintandrewhall · 2024-11-22T05:09:36Z

Some className attributes have been kept, because they are referenced in JS and tests

If it still applies, if class names are referenced in tests, convert to data-test-subj.

tsullivan · 2024-11-22T15:26:40Z

@clintandrewhall

If it still applies, if class names are referenced in tests, convert to data-test-subj.

That part of the description is outdated. I will update.

See also the thread here: #199885 (comment)

tsullivan added 2 commits November 12, 2024 12:51

Use Emotion for loading-indicator CSS

73bb3a8

Use Emotion for header-logo CSS

5f6cede

tsullivan requested review from a team as code owners November 12, 2024 22:47

Use Emotion for collapsible-nav CSS

d3fe030

tsullivan force-pushed the remove-sass/core-chrome-browser-internal branch from 0c39420 to d3fe030 Compare November 12, 2024 22:50

tsullivan commented Nov 12, 2024

View reviewed changes

Use “object styles” syntax

3ce08ce

tsullivan changed the title ~~[SharedUX] Replace Sass with Emotion, round 1~~ [SharedUX] Replace Sass with Emotion, Round 1 Nov 12, 2024

tsullivan added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Nov 12, 2024

tsullivan added 6 commits November 13, 2024 10:08

Merge branch 'main' into remove-sass/core-chrome-browser-internal

d695c41

Restore className props

ae46116

update test snapshots

4377bee

Remove unused scss

1779bbb

remove classNames that are not needed

134052b

restore styles to stop global header buttons from jumping during load…

8e4cd1d

…ing state

tsullivan commented Nov 13, 2024

View reviewed changes

ryankeairns self-requested a review November 13, 2024 22:57

ryankeairns approved these changes Nov 13, 2024

View reviewed changes

tsullivan and others added 3 commits November 14, 2024 13:30

Merge branch 'main' into remove-sass/core-chrome-browser-internal

1d440a7

fix enzyme test

0f6006c

Merge branch 'main' into remove-sass/core-chrome-browser-internal

1229f05

eokoneyo reviewed Nov 20, 2024

View reviewed changes

tsullivan added 5 commits November 20, 2024 12:17

Merge branch 'main' into remove-sass/core-chrome-browser-internal

4473066

replace className with data-test-subj

bb45851

Merge branch 'main' into remove-sass/core-chrome-browser-internal

1ada42c

update snapshot

8b027a0

Remove chrHeaderLogo className

24f6977

tsullivan requested a review from a team as a code owner November 21, 2024 23:20

tsullivan commented Nov 21, 2024

View reviewed changes

Merge branch 'main' into remove-sass/core-chrome-browser-internal

9395489

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SharedUX] Replace Sass with Emotion, Round 1 #199885

[SharedUX] Replace Sass with Emotion, Round 1 #199885

tsullivan commented Nov 12, 2024 •

edited

Loading

tsullivan Nov 12, 2024

ryankeairns Nov 13, 2024

ryankeairns Nov 13, 2024

tsullivan Nov 13, 2024

tsullivan Nov 13, 2024

eokoneyo Nov 20, 2024

tsullivan Nov 20, 2024 •

edited

Loading

tsullivan Nov 20, 2024

logeekal Nov 22, 2024

tsullivan Nov 22, 2024

tsullivan Nov 21, 2024

logeekal Nov 22, 2024

kqualters-elastic Nov 25, 2024

tsullivan Nov 25, 2024 •

edited

Loading

tsullivan commented Nov 21, 2024

elasticmachine commented Nov 22, 2024

clintandrewhall commented Nov 22, 2024

tsullivan commented Nov 22, 2024

[SharedUX] Replace Sass with Emotion, Round 1 #199885

Are you sure you want to change the base?

[SharedUX] Replace Sass with Emotion, Round 1 #199885

Conversation

tsullivan commented Nov 12, 2024 • edited Loading

Summary

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

tsullivan commented Nov 21, 2024

elasticmachine commented Nov 22, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

clintandrewhall commented Nov 22, 2024

tsullivan commented Nov 22, 2024

tsullivan commented Nov 12, 2024 •

edited

Loading

tsullivan Nov 20, 2024 •

edited

Loading

tsullivan Nov 25, 2024 •

edited

Loading