Proposed minor CSS variable cleanup #8
Merged
Conversation
This file contains 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
…instead + separate `kbnBody--projectActionMenuVisible` into its own selector, to improve readability and reduce CSS specificity
…iFixedHeadersOffset` if not set
cee-chen
commented
Oct 11, 2023
Comment on lines
-10
to
-11
| // total height of all fixed headers for the app container in serverless projects, dynamically changes based on whether the banner or action menu is present | ||
| --kbnAppHeadersOffset: var(--euiFixedHeadersOffset, 0); |
There was a problem hiding this comment.
Removing this from :root is a minor cleanup preference. IMO, this variable only needs to get set on the appropriate body modifier. Other CSS can conditionally use either this variable or --euiFixedHeadersOffset if present by simply nesting var() fallbacks:
cee-chen
commented
Oct 11, 2023
Comment on lines
+59
to
+61
| // Set a body CSS variable for the app container to use - calculates the total | ||
| // height of all fixed headers + the sticky action menu toolbar | ||
| .kbnBody--projectActionMenuVisible { |
There was a problem hiding this comment.
I pulled this out to its own selector block instead of bogarting the other existing selectors because I thought it was easier to read and understand separately. If the toolbar goes away or changes its positioning logic someday, separating it also makes it easier to delete all at once
cee-chen
commented
Oct 11, 2023
Comment on lines
76
to
78
| &.kbnBody--projectActionMenuVisible { | ||
| --kbnAppHeadersOffset: var(--kbnHeaderBannerHeight); | ||
| --kbnAppHeadersOffset: var(--euiFixedHeadersOffset, 0); | ||
| } |
There was a problem hiding this comment.
This I kept in the .kbnBody--chromeHidden block because that is a pretty special state that needs to come after all other (in terms of CSS specificity). I also DRY'd it out to simply inherit straight from --euiFixedHeadersOffset, which should be set properly from the preceding 6 lines
Dosant
merged commit 39d62e2
into
Dosant:d/2023-10-09-fix-sticky-header
1 of 2 checks passed
elasticmachine
pushed a commit
that referenced
this pull request
Sep 2, 2024
fixes [#8](elastic/observability-accessibility#8) fixes [#7](elastic/observability-accessibility#7) ## Summary Fixes APM breadcrumbs on serverless | Serverless | Stateful | |---|---| | <img width="700px" alt="image" src="https://github.com/user-attachments/assets/944a7d58-7de3-4a7f-be02-3c8c1110a0e2"> |<img width="800px" alt="image" src="https://github.com/user-attachments/assets/450664b1-ddfc-4395-9fa3-a7b941affb3b">| |<img width="500px" alt="image" src="https://github.com/user-attachments/assets/944a7d58-7de3-4a7f-be02-3c8c1110a0e2"> |<img width="500px" alt="image" src="https://github.com/user-attachments/assets/450664b1-ddfc-4395-9fa3-a7b941affb3b">| | <img width="500px" alt="image" src="https://github.com/user-attachments/assets/944a7d58-7de3-4a7f-be02-3c8c1110a0e2"> |<img width="500px" alt="image" src="https://github.com/user-attachments/assets/cb8a39e2-ca33-4cf9-a8ac-4c84566d092d">| |<img width="500px" alt="image" src="https://github.com/user-attachments/assets/151a3a9c-c81e-4558-9d00-e695e3d1d79c">|<img width="500px" alt="image" src="https://github.com/user-attachments/assets/2562e96f-d5e4-4aa4-a221-6721f8995883">| |<img width="500px" alt="image" src="https://github.com/user-attachments/assets/8d877d11-8c3f-4ac5-8146-6a11125eae7c">|<img width="500px" alt="image" src="https://github.com/user-attachments/assets/36e588cb-4c18-4d66-a2c6-f0e66392f708">| |<img width="500px" alt="image" src="https://github.com/user-attachments/assets/14253196-06de-4343-811f-61aa31ea0d1e">|<img width="500px" alt="image" src="https://github.com/user-attachments/assets/0cdfc83f-6545-433f-8c14-5bbf2a581175">| |<img width="500px" alt="image" src="https://github.com/user-attachments/assets/89a58e2b-2cef-4188-b2be-f359ba6890db">|<img width="500px" alt="image" src="https://github.com/user-attachments/assets/f15e767f-5b60-4485-ac71-7b6fd850ec50">| |<img width="500px" alt="image" src="https://github.com/user-attachments/assets/a0f7bfae-bfda-4f49-b92a-e736d80fea4c">|<img width="500px" alt="image" src="https://github.com/user-attachments/assets/680db8ab-58b8-454b-a0d7-6e1681dbe616">| ### How to test #### Serverless - Start a local ES serverless instance: `yarn es serverless --projectType=oblt --ssl -k/--insecure` - Start a local Kibana serverless instance: ` yarn start --serverless=oblt --no-ssl` - Run some synthtrace scenarios - `NODE_TLS_REJECT_UNAUTHORIZED=0 node scripts/synthtrace mobile.ts --live --target=https://elastic_serverless:changeme@127.0.0.1:9200 --kibana=http://elastic_serverless:changeme@0.0.0.0:5601` - `NODE_TLS_REJECT_UNAUTHORIZED=0 node scripts/synthtrace service_map.ts --live --target=https://elastic_serverless:changeme@127.0.0.1:9200 --kibana=http://elastic_serverless:changeme@0.0.0.0:5601` - Navigate to Applications and click through the links ### Stateful - Start a local ES and Kibana instance - Run the some synthtrace scenarios: - `node scripts/synthtrace mobile.ts --live` - `node scripts/synthtrace service_map.ts --live` - Navigate to Applications and click through the links --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
👋 Hey Anton! Thought I'd throw a CSS cleanup suggestion your way for elastic#168372. The goal is to reduce CSS specificity and try to improve readability somewhat, although the latter is always a bit subjective. LMK if you have any questions!