[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!

Thanks I will test this tomorrow..

Okay I did some desk testing with the new change and there are couple of issues with this change.

Although body is focusable, it is not tabbable which actually takes user out of tab order and if once the focus is reseton body, pressing Tab after that does nothing.

In previous case, the reset was focusing the logo, which was kind of first in the tab order and users can now tab through out the page which is not possible in the new change.

It looks like, this function ( old version, not the change) is broken in serverless, since logo with class chrHeaderLogo does not even exists in Serverless.

this begs the question, if we even need this functionality. I will discuss with the team today and if we do not, i will push the commit to remove it.

Thanks @logeekal!

@logeekal Does this PR need to be held on the discussion with your team? I hope not, and I think that if nothing else is blocking this PR, it could get approved and the team discussion could be handled as follow-up work.

Does that sound ok?

@tsullivan , Sorry for the delay from my side. It took time to get full information, but i can say that this piece of code is not required at our end. But I will create a separate PR to remove it so that you are not blocked.

I will create a separate PR to add that code. Thanks for your patience.

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

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

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)

logeekal

investigations LGTM 🚀 . Thanks for answering all our queries.

elasticmachine · 2024-12-04T08:50:26Z

💚 Build Succeeded

Buildkite Build
Commit: 0cd19de

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`	14.6MB	14.6MB	-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.6KB	450.2KB	-4.4KB

History

💛 Build #253403 was flaky 24f6977
💔 Build #252944 failed bb45851
💚 Build #252025 succeeded 1229f05
💔 Build #251574 failed 0f6006c
💔 Build #250991 failed 8e4cd1d
💔 Build #250639 failed 3ce08ce

eokoneyo

LGTM

kibanamachine · 2024-12-04T17:39:39Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12165272922

kibanamachine · 2024-12-04T17:44:46Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts You might need to backport the following PRs to 8.x: - [Synthetics] Fix overview trends for read-only user !! (#202914) - [EDR Workflows] Endpoint Insights API (#201521) - [React18] Migrate test suites to account for testing library upgrades security-threat-hunting-explore (#201142) - [Rules migration] Post merge feedback followups (#202815) - [ResponseOps][Cases] Fix edit cases settings privilege (#202053) - Check for indices before enabling get search profile in serverless (#201630) - [Response Ops][Alerting] Alerting event logger should not throw error when ad hoc task runner cannot load saved object (#201637) - [RCA] [Events timeline] Show all events (#202235) - Enhance access query logic to handle user ID and name conditions (#202833) - [EEM] Add create and read APIs for v2 type and source definitions (#201470)

Manual backport

To create the backport manually run:

node scripts/backport --pr 199885

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2024-12-05T17:49:10Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199885 locally

Part of elastic/kibana-team#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. 1. https://emotion.sh/docs/globals 2. https://emotion.sh/docs/best-practices 3. elastic/eui#6828 (comment) --------- Co-authored-by: Jatin Kathuria <[email protected]> (cherry picked from commit d86896b)

tsullivan · 2024-12-06T18:33:45Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

## Summary Part of elastic/kibana-team#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 1. https://emotion.sh/docs/globals 2. https://emotion.sh/docs/best-practices 3. elastic/eui#6828 (comment) --------- Co-authored-by: Jatin Kathuria <[email protected]>

# Backport This will backport the following commits from `main` to `8.x`: - [[SharedUX] Replace Sass with Emotion, Round 1 (#199885)](#199885)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

## Summary Part of elastic/kibana-team#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 1. https://emotion.sh/docs/globals 2. https://emotion.sh/docs/best-practices 3. elastic/eui#6828 (comment) --------- Co-authored-by: Jatin Kathuria <[email protected]>

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

logeekal approved these changes Dec 4, 2024

View reviewed changes

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

0cd19de

eokoneyo approved these changes Dec 4, 2024

View reviewed changes

tsullivan merged commit d86896b into elastic:main Dec 4, 2024
9 checks passed

tsullivan deleted the remove-sass/core-chrome-browser-internal branch December 4, 2024 17:39

kibanamachine added the v9.0.0 label Dec 4, 2024

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 5, 2024

tsullivan added the v8.18.0 label Dec 6, 2024

tsullivan mentioned this pull request Dec 6, 2024

[8.x] [SharedUX] Replace Sass with Emotion, Round 1 (#199885) #203314

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 9, 2024

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 by kibanamachine

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

logeekal Nov 26, 2024

logeekal Nov 27, 2024

tsullivan Nov 27, 2024

tsullivan Dec 3, 2024

logeekal Dec 4, 2024

tsullivan commented Nov 21, 2024

clintandrewhall commented Nov 22, 2024

tsullivan commented Nov 22, 2024

logeekal left a comment

elasticmachine commented Dec 4, 2024

eokoneyo left a comment

kibanamachine commented Dec 4, 2024

kibanamachine commented Dec 4, 2024

kibanamachine commented Dec 5, 2024

tsullivan commented Dec 6, 2024

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

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

Conversation

tsullivan commented Nov 12, 2024 • edited by kibanamachine 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

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 commented Nov 21, 2024

clintandrewhall commented Nov 22, 2024

tsullivan commented Nov 22, 2024

logeekal left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 4, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

eokoneyo left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 4, 2024

kibanamachine commented Dec 4, 2024

💔 All backports failed

Manual backport

Questions ?

kibanamachine commented Dec 5, 2024

tsullivan commented Dec 6, 2024

💚 All backports created successfully

Questions ?

tsullivan commented Nov 12, 2024 •

edited by kibanamachine

Loading

tsullivan Nov 20, 2024 •

edited

Loading

tsullivan Nov 25, 2024 •

edited

Loading