Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens] Align Lens style with Borealis #204839

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

markov00
Copy link
Member

Summary

This PR updates the style of Lens to align it with the new Borealis theme.
It covers the set of tasks in #203050 within the Lens editor panels list.

Comments are applied in order as in the mentioned issue

@markov00 markov00 added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Lens backport:skip This commit does not require backporting v9.0.0 EUI Visual Refresh labels Dec 18, 2024
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 18, 2024

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Adding an early review due to upcoming PTO.

@@ -79,7 +79,7 @@ export function ColorPicker({
close();
deleteStep();
}}
style={{ paddingBottom: 8 }}
css={{ paddingBottom: 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: We could also use a token here instead of a static value.

css={({ euiTheme }) => ({
  paddingBottom: euiTheme.size.s
})}

ℹ️ In case this causes a typescript error on the euiTheme not being defined, see this related comment on required steps.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in f33c9ff

const selectedColorSwatchStyle = {
outline: 'currentcolor solid 2px',
outlineOffset: '-1px',
border: `2px solid ${euiTheme.colors.borderBaseFormsColorSwatch}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: We could use euiTheme.border.width.thick here instead of 2px.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in f33c9ff

gap: ${euiTheme.size.s};
min-height: ${euiTheme.size.xl};
padding: ${euiTheme.size.xs} ${euiTheme.size.s};
background-color: ${euiTheme.colors.backgroundBaseSubdued};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what this renders, but based on color value mappings alone, this might rather be using backgroundBasePlain.
Feel free to ignore if this is already expected as is 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This color was specified by @MichaelMarcialis

>
<div
css={css`
max-width: 400px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Just fyi, once this EUI upgrade PR is merged, you could use euiTheme.components.forms.maxWidth.

@@ -363,7 +363,7 @@ export function LayerPanel(props: LayerPanelProps) {
className="lnsLayerPanel"
data-test-subj={`lns-layerPanel-${layerIndex}`}
>
<EuiPanel paddingSize="none">
<EuiPanel paddingSize="none" hasShadow={false} hasBorder={true}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, this could use shorthand: hasBorder instead of hasBorder={true}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, done in f33c9ff

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
discover 786.7KB 786.7KB +1.0B
eventAnnotationListing 232.2KB 232.1KB -78.0B
lens 1.5MB 1.5MB +926.0B
securitySolution 21.4MB 21.4MB +1.0B
total +850.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/coloring 2 3 +1

Total ESLint disabled count

id before after diff
@kbn/coloring 2 3 +1

History

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting EUI Visual Refresh Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants