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

Migration to emotionjs #4123

Merged
merged 26 commits into from
Apr 9, 2024
Merged

Migration to emotionjs #4123

merged 26 commits into from
Apr 9, 2024

Conversation

teodosii
Copy link
Member

What this PR does

Migrate existing CSS/SCSS to emotion

Which issue(s) this PR closes

Closes #2666

@teodosii teodosii marked this pull request as ready for review April 8, 2024 07:48
@teodosii teodosii requested a review from a team as a code owner April 8, 2024 07:48
@teodosii teodosii added pr:no public docs Added to a PR that does not require public documentation updates release:ignore PR will not be added to release notes labels Apr 8, 2024
this.styles = getEscalationPolicyStyles(this.props.theme);
this.forceUpdate();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think just as in functional components we could just define it in render like

render() {
  const styles = getEscalationPolicyStyles(props.theme);
  ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, ideally styles should be just in render body and nowhere else

Copy link
Member Author

Choose a reason for hiding this comment

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

Too many render functions, that means 1 line for each subrender where we're using the styles... not worth it, it takes 3 lines with cDU lifecycle.

import CopyToClipboard from 'react-copy-to-clipboard';

import { Block } from 'components/GBlock/Block';
import { Text } from 'components/Text/Text';
import { openNotification } from 'utils/utils';
import { bem, openNotification } from 'utils/utils';
import { getUtilStyles } from 'utils/utils.styles';
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe better create a dedicate folder for styles like grafana-plugin/src/styles

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea

className={cx('integrationBlock__heading', {
'integrationBlock__heading--noBorderBottom': !noContent,
className={cx(styles.integrationBlockHeading, {
[bem(styles.integrationBlockHeading, 'noBorderBottom')]: !noContent,
Copy link
Contributor

@maskin25 maskin25 Apr 8, 2024

Choose a reason for hiding this comment

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

 className={cx(styles.integrationBlockHeading, {
            [bem(styles.integrationBlockHeading, 'noBorderBottom')]: !noContent
})}

looks too ease to make typos, how does grafana core deal with nested selectors? I'm pretty sure it does not use bem.

And in general styling looks overcomplicated, to style something we need too much stuff to import

import { cx } from '@emotion/css';
import { useStyles2 } from '@grafana/ui';
import { bem } from 'utils/utils';
import { getIntegrationBlockStyles } from './IntegrationBlock.styles';

Can we remove at least one thing? the most suitable candidate seems to be a bem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Grafana doesn't use BEM pattern. Why remove any of it? I don't understand your reasons...

cx is for composition
useStyles2 is mandatory ...
bem is too build line blocks following BEM pattern so that you don't need to repeat the block, but instead rely on a helper that does that. BEM is very useful because it clearly expresses what the element modifier is supposed to do.
getIntegrationBlockStyles is outside the file so that we don't have too much code in the main file

I strongly recommend reading on https://getbem.com/

Copy link
Member Author

@teodosii teodosii Apr 9, 2024

Choose a reason for hiding this comment

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

The code above has more in it because it depends on a conditional, is just like the old compose cx

styles.integrationBlockHeading gets appended
integrationBlockHeading--noBorderBottom' gets appended if noContent is false/undefined

lint
@teodosii teodosii added this pull request to the merge queue Apr 9, 2024
Merged via the queue into dev with commit 45d0390 Apr 9, 2024
21 checks passed
@teodosii teodosii deleted the rares/emotionjs-migration branch April 9, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates release:ignore PR will not be added to release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt EmotionJS usage in OnCall to allow better integration with grafana themes
3 participants