[CSS-in-JS] Convert EuiMark
#4575
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import { css } from '@emotion/react'; | ||
| import { UseEuiTheme, transparentize } from '../../services'; | ||
|
|
||
| export const euiMarkStyles = ({ euiTheme, colorMode }: UseEuiTheme) => { | ||
|
There was a problem hiding this comment. Result of |
||
| // TODO: Was $euiFocusBackgroundColor | ||
| const transparency = { LIGHT: 0.1, DARK: 0.3 }; | ||
cee-chen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return css` | ||
| background-color: ${transparentize( | ||
| euiTheme.colors.primary, | ||
| transparency[colorMode] | ||
| )}; | ||
| font-weight: ${euiTheme.font.weight.bold}; | ||
| // Override the browser's black color. | ||
| // Can't use 'inherit' because the text to background color contrast may not be sufficient | ||
| color: ${euiTheme.colors.text}; | ||
| `; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,10 @@ | |
| */ | ||
|
|
||
| import React, { HTMLAttributes, FunctionComponent, ReactNode } from 'react'; | ||
| import classNames from 'classnames'; | ||
| import { CommonProps } from '../common'; | ||
| import { useEuiTheme } from '../../services'; | ||
| import { euiMarkStyles } from './mark.styles'; | ||
| export type EuiMarkProps = HTMLAttributes<HTMLElement> & | ||
| CommonProps & { | ||
| /** | ||
|
|
@@ -22,10 +24,12 @@ export const EuiMark: FunctionComponent<EuiMarkProps> = ({ | |
| className, | ||
| ...rest | ||
| }) => { | ||
| const useTheme = useEuiTheme(); | ||
| const styles = euiMarkStyles(useTheme); | ||
|
Comment on lines
+27
to
+28
There was a problem hiding this comment. DevEx question: why pass a hook into a separate file? Why not just make There was a problem hiding this comment. Sorry, to be clear, I'm proposing this instead: // mark.tsx
const styles = useEuiMarkStyles();
// mark.styles.ts
import { css } from '@emotion/react';
import { useEuiTheme, transparentize } from '../../services';
export const useEuiMarkStyles = () => {
const { euiTheme, colorMode } = useEuiTheme();
return css `// ... etc`;
};There was a problem hiding this comment. Ah sorry, I see you addressed this in https://github.com/elastic/eui/pull/4575/files#r808436110 - I'm still not totally clear on the rationale behind that comment though. Is it a bad thing for a style file to be dependent on React context? TBH, keeping our usage of EUI theming to the style-focused file makes more sense to me from a developer POV. I'm also very curious what usage would look like with a traditional class component (i.e.: can't use hooks) vs. a functional component. Do we have an example of that? There was a problem hiding this comment. Did this so that it's possible to just pass in a theme shaped object and get styles, without needing the full React context. No reason we can't do a two-step thing for internal use. That is, keep There was a problem hiding this comment. In more complex cases, we'll probably need to have There was a problem hiding this comment. Gotcha! I think the benefits will be clearer with a dynamic styles example + a class component example (I assume we will need to use a HOC for class components). If we could add those examples to the TBD wiki section along with a basic example that would be amazing! |
||
| const classes = classNames('euiMark', className); | ||
|
|
||
| return ( | ||
| <mark css={[styles]} className={classes} {...rest}> | ||
|
There was a problem hiding this comment. Ok so the output class list is now: Seems like a lot of variations of There was a problem hiding this comment. Right, we can reduce the verbosity if deemed necessary. These are only referential so no changes will ever be breaking. |
||
| {children} | ||
| </mark> | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the long-form class name defined by
labelFormatin dev and prod environments (prod would use a short, mangled string otherwise).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little confused here because you mention "in dev and prod environments" but then say that prod would use the short/uglified string. Just to clarify, the long-form string is in dev only, and the short-form string is prod only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The properties are confusing, yes.
"autoLabel": "always"makes it so that the long-form string is used in both prod and dev environments. We don't want to use the short-form string in any case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh my reading comprehension is at an all time low today 🤦 Sorry, I totally missed the "otherwise" in that parentheses! Alrighty, I get it now, so always a long-form string.