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

[Emotion] Improve checklist/QA process #6294

Merged
merged 4 commits into from
Oct 7, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
### Summary
## Summary

Provide a detailed summary of your PR. Explain how you arrived at your solution. If it includes changes to UI elements include a screenshot or gif.

### Checklist
## QA

Remove or strikethrough items that do not apply to your PR.

### General checklist

- [ ] Checked in both **light and dark** modes
- [ ] Checked in **mobile**
Expand All @@ -15,3 +19,47 @@ Provide a detailed summary of your PR. Explain how you arrived at your solution.
- [ ] Checked for **accessibility** including keyboard-only and screenreader modes
- [ ] Updated the **[Figma](https://www.figma.com/community/file/964536385682658129)** library counterpart
- [ ] A **[changelog](https://github.com/elastic/eui/blob/main/wiki/documentation-guidelines.md#changelog)** entry exists and is marked appropriately

### Emotion conversion checklist

- **Does it work?**
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
- [ ] Output CSS matches the previous CSS / as expected in browsers
- [ ] Rendered className reads as expected in snapshots and in browsers
- [ ] Checked component playground (class components wrapped in `withEuiTheme` need to pass `true` as the second argument to its `propUtilityForPlayground` in `src-docs/src/views/{component}/playground.js`)
 
- **Unit tests**
- [ ] [`shouldRenderCustomStyles()`](https://github.com/elastic/eui/blob/6054e9b8310bdb106371c0c9ff8bc48e3e0e594b/src/test/internal/render_custom_styles.tsx) test was added and passes with parent component and any nested `childProps` (e.g. `tooltipProps`)
- [ ] Removed any `mount()`ed snapshots in favor of `render()` or a more specific assertion
 
- **Sass/Emotion conversion process**
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
- [ ] Converted all global Sass vars/mixins to JS (e.g. `$euiSize` to `euiTheme.size.base`)
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
- [ ] Removed or converted component-specific Sass vars/mixins to exported JS versions, **listed removals in changelog**, and ran `yarn compile-scss` to update [JSON files](https://github.com/elastic/eui/tree/main/src-docs/src/views/theme/_json)
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
- [ ] Simplified `calc()` to `mathWithUnits` if possible (if mixing different unit types, this may not be possible)
- [ ] Added an `@warn` deprecation message within the `global_styling/mixins/{component}.scss` file
- [ ] Removed component from `src/components/index.scss`
- [ ] Deleted any `src/amsterdam/overrides/{component}.scss` files (styles within should have been converted to the baseline Emotion styles)
 
- **CSS tech debt**
- [ ] Reduced specificity where possible (usually by reducing nesting and class name chaining)
- [ ] Wrapped all animations or transitions in `euiCanAnimate`
- [ ] Used `gap` property to add margin between items if using flex
- [ ] Converted side specific padding, margin, and position to `-inline` and `-block` [logical properties](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Logical_Properties) (check inline styles as well as CSS)
 
- **DOM cleanup**
- [ ] Did **not** remove any block/element classNames (e.g. `euiComponent`, `euiComponent__child`)
- [ ] Deleted any modifier classNames or maps if not being used in Kibana. **Before doing this step**:
- [ ] Search/grep through Kibana's codebase for `{euiComponent}-` (case sensitive) to check for source code usage of modifier classes
- [ ] If usages exist, consider converting to a `data` attribute so that consumers still have something to hook into
 
- **Kibana due diligence**
- Pre-emptively check how your conversion will impact the next Kibana upgrade. This entails searching/grepping through Kibana (excluding `**/target, **/*.snap, **/*.storyshot` for less noise) for `eui{Component}` (case sensitive) to find:
- [ ] Any test/query selectors that will need to be updated
- [ ] Any Sass or CSS that will need to be updated, particularly if a component Sass var was deleted
- [ ] Any direct className usages that will need to be refactored (e.g. someone calling the `euiBadge` class on a div instead of simply using the `EuiBadge` component)
 
- **Extras/nice-to-have**
- [ ] Documentation pass:
- [ ] Converted any remaining `.js` docs files to TS
- [ ] Misc cleanup of docs code (e.g. combine imports to single `from '../src'`, replace `<React.Fragment>` with `<>`)
- [ ] Check for issues in the backlog that could be a quick fix for that component
- [ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about
6 changes: 6 additions & 0 deletions generator-eui/changelog/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ module.exports = class extends Generator {
type: 'confirm',
default: false,
},
{
message: 'Does your PR contain Emotion conversions?',
name: 'emotionConversions',
type: 'confirm',
default: false,
},
];

return this.prompt(prompts).then((answers) => {
Expand Down
6 changes: 6 additions & 0 deletions generator-eui/changelog/templates/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,10 @@
**Breaking changes**

- Removed ...

<%_ } -%>
<%_ if (emotionConversions) { -%>
**CSS-in-JS conversions**

- Converted `EuiComponent` to Emotion; Removed `$euiComponentSassVariable`
<%_ } -%>
4 changes: 4 additions & 0 deletions upcoming_changelogs/_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,7 @@
**Breaking changes**

- Removed `EuiComponentD`

**CSS-in-JS conversions**

- Converted `EuiComponent` to Emotion; Removed `$euiComponentSassVariable`