Import space selector styles #64656
Import space selector styles #64656
Conversation
legrego
added
Team:Security
|
Pinging @elastic/kibana-security (Team:Security) |
| @@ -4,6 +4,7 @@ | |||
| * you may not use this file except in compliance with the Elastic License. | |||
| */ | |||
|
|
|||
| import './nav_control.scss'; | |||
There was a problem hiding this comment.
This doesn't seem like the appropriate file to import this from. This file is for a popover, but the nav_control.scss file contains styles for the whole page?
There was a problem hiding this comment.
Good call, I think the nav_control component is likely the best place to do this import
| @@ -4,6 +4,8 @@ | |||
| * you may not use this file except in compliance with the Elastic License. | |||
| */ | |||
|
|
|||
| import './_index.scss'; | |||
There was a problem hiding this comment.
Does this index file still need to be imported in this sass file anymore?
There was a problem hiding this comment.
I suspect not, this is what got us into trouble in the first place 😄
| @import './components/index'; | ||
| @import './nav_control'; |
There was a problem hiding this comment.
This was actually still the correct structure. Imports should happen in a single file.
There was a problem hiding this comment.
I was trying to follow the advice outlined here: #64511 (review)
The top-level import is the nav_control.scss file, which itself contains the single import for this view. Am I thinking about this the wrong way?
There was a problem hiding this comment.
We try to follow the pattern that individual component_name.scss files should not import lower level components. Only index.scss should import multiple files, or a full plugin's worth of files. This helps us understand the underlying structure SASS structure without having to open each file to know what is importing what.
Yes it is correct that now we can import each component's file into it's corresponding .tsx file. However, it should be using only one of these import techniques and not a combination of both:
- One
index.scssmanifest file that imports individual component_component.scssfiles, and gets itself imported in the topmost component asimport './index.scss' - Every component has a corresponding
component.scssfile that is imported directly by the component'scomponent.tsxfile. Meaning, lower level component SASS files likenav_control/components/spaces_description.scssshould be imported insidenav_control/components/spaces_description.tsx. And not at the top levelnav_control.scss.
I know we're in a weird transition phase right now, which is why it's all that more important that we either stick to the ole _index.scss manifest if you need to ensure all styles are imported by a top level component, or fully switch to the more modular import structure where each JS component imports its own SCSS file.
There was a problem hiding this comment.
@legrego I created a PR for you to show the new import structure for the modularized technique. legrego#24
There was a problem hiding this comment.
@cchaos thanks for the explanation, that helped clarify things for me. Thanks also for the PR, that was a nice surprise!
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Summary
#61011 cleaned up legacy imports, but in the process ended up breaking the style of the space selector ui:
The space selector screen was incorrectly relying on legacy styles, rather than importing its own set of styles. This PR adds that import to correct the display: