[SharedUX] SCSS migration Home plugin #214859
Conversation
paulinashakirova
added
release_note:skip
| import { METRIC_TYPE } from '@kbn/analytics'; | ||
| import { FormattedMessage } from '@kbn/i18n-react'; | ||
| import { css } from '@emotion/react'; | ||
| import { fullScreenGraphicsMixinStyles } from '../kibanaFullScreenGraphicsMixin'; |
There was a problem hiding this comment.
This is a locally defined "mixin" that can potentially be defined and imported from a core-app where there .scss mixin used to be.
| /** | ||
| * Shows a full-screen welcome page that gives helpful quick links to beginners. | ||
| */ | ||
| export class Welcome extends React.Component<Props> { | ||
| private services = getServices(); | ||
| export const Welcome: React.FC<WelcomeProps> = ({ urlBasePath, onSkip }: WelcomeProps) => { |
There was a problem hiding this comment.
This change is a bit tricky - I can revert if it's better so as to not mix oranges and apples...
I refactored this component class->function because I wanted to use useEuiShadow, though if it's out of scope and I should look for an alternative approach - i ll do so 😊
| Number(useEuiTheme().euiTheme.levels.navigation), | ||
| useEuiTheme() | ||
| ), | ||
| ]} |
There was a problem hiding this comment.
Also not sure if there is a better approach to "calling" these mixin styles than this.
There was a problem hiding this comment.
I think we'd probably get a lint error here for rules of hook, lets invoke the useEuiTheme hook outside of the render and pass the value we want to the mixin
src/platform/plugins/shared/home/public/application/components/manage_data/manage_data.tsx
Show resolved
Hide resolved
paulinashakirova
changed the title
src/platform/plugins/shared/home/public/application/components/manage_data/manage_data.tsx
Show resolved
Hide resolved
| <EuiButton | ||
| fill | ||
| css={({ euiTheme }: UseEuiTheme) => | ||
| css({ | ||
| marginRight: euiTheme.size.s, | ||
| }) | ||
| } | ||
| onClick={onConfirm} | ||
| > | ||
| <FormattedMessage id="home.tryButtonLabel" defaultMessage="Add integrations" /> | ||
| </EuiButton> | ||
| <EuiButtonEmpty | ||
| className="homWelcome__footerAction" | ||
| css={({ euiTheme }: UseEuiTheme) => | ||
| css({ | ||
| marginRight: euiTheme.size.s, | ||
| }) | ||
| } |
There was a problem hiding this comment.
We could consolidate these two definitions into a single declaration that are then referenced for brevity, also it means only one style will be generated instead of two. Maybe something like so;
| <EuiButton | |
| fill | |
| css={({ euiTheme }: UseEuiTheme) => | |
| css({ | |
| marginRight: euiTheme.size.s, | |
| }) | |
| } | |
| onClick={onConfirm} | |
| > | |
| <FormattedMessage id="home.tryButtonLabel" defaultMessage="Add integrations" /> | |
| </EuiButton> | |
| <EuiButtonEmpty | |
| className="homWelcome__footerAction" | |
| css={({ euiTheme }: UseEuiTheme) => | |
| css({ | |
| marginRight: euiTheme.size.s, | |
| }) | |
| } | |
| const footerAction = useCallback(({ euiTheme }: UseEuiTheme) => { | |
| return css({ | |
| marginRight: euiTheme.size.s, | |
| }) | |
| }, [euiTheme]); | |
| ... | |
| <EuiButtonEmpty fill css={footerAction} ... | |
| > | ||
| <EuiScreenReaderOnly> | ||
| <h2 id="homSolutions__title"> | ||
| <h2 id="homeSolutions__title"> |
There was a problem hiding this comment.
I fear these changes might increase the scope of this PR, especially if these ids are used in either ftr or functional tests
There was a problem hiding this comment.
Yesss, I checked with those tests and updated accordingly 😊
| Number(useEuiTheme().euiTheme.levels.navigation), | ||
| useEuiTheme() | ||
| ), | ||
| ]} |
There was a problem hiding this comment.
I think we'd probably get a lint error here for rules of hook, lets invoke the useEuiTheme hook outside of the render and pass the value we want to the mixin
| backgroundColor: euiTheme.euiTheme.colors.backgroundBasePlain, | ||
| opacity: 0, | ||
| overflow: 'auto', | ||
| animation: `${fullScreenGraphicsFadeIn} ${euiTheme.euiTheme.animation.extraSlow} ${euiTheme.euiTheme.animation.resistance} 0s forwards`, |
There was a problem hiding this comment.
We typically want to wrap animation declarations within the canAnimate helper util from eui, see here
⏳ Build in-progress, with failures
Failed CI StepsTest Failures
History
|
| @@ -30,7 +30,6 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { | |||
| expect(event.properties.target).to.be.an('array'); | |||
| const targets = event.properties.target as string[]; | |||
| expect(targets.includes('DIV')).to.be(true); | |||
| expect(targets.includes('class=homWelcome')).to.be(true); | |||
There was a problem hiding this comment.
I removed this class name as it is no longer used in the code.
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
paulinashakirova
changed the title
|
@paulinashakirova I recently updated the login page to use the same svgs... is it technically possible to reuse those from core versus duplicating them in this plugin directory? The are here: https://github.com/elastic/kibana/tree/main/src/core/public/styles/core_app/images |
@ryankeairns |
|
Oh, also, you can then remove the .svg files from this PR then, correct? |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsmiscellaneous assets size
History
|
|
Starting backport for target branches: 9.0 |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
Summary
This PR is a part of a Replace SCSS with CSS-in-JS epic.
@emotion/react