Skip to content

Commit

Permalink
[Emotion] Improve checklist/QA process (#6294)
Browse files Browse the repository at this point in the history
* Update GH pull request template to ensure Emotion conversion checklist is not skipped

+ expand/categorize checklist items

* Add Emotion conversions as a changelog template for more consistency

* PR feedback

* Remove unnecessary bolding

The template update takes care of ensuring we list Sass removals
  • Loading branch information
Constance authored Oct 7, 2022
1 parent 1da9435 commit 1e19586
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 2 deletions.
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?**
- [ ] 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**
- [ ] Converted all global Sass vars/mixins to JS (e.g. `$euiSize` to `euiTheme.size.base`)
- [ ] 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)
- [ ] 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`

0 comments on commit 1e19586

Please sign in to comment.