-
Notifications
You must be signed in to change notification settings - Fork 841
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
Conversation
…t is not skipped + expand/categorize checklist items
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6294/ |
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.
Feedback would be appreciated on the PR template md file (tagging multiple reviewers for that purpose). Are items well categorized? Would you know how to know if a step should or shouldn't be done with just the information provided, or is more context needed?
The items are well categorized and I would know if a step should or shouldn't be done with just the information provided. So it LGTM! 👍🏽
For me, the only boring thing when I'm going through the checklist is to strikethrough. Removing items eliminates the context of what doesn't apply. So I always prefer to strikethrough. But I wonder if anyone has an alternative to the strikethrough.
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.
Thank you for putting this thorough update together @constancecchen. My comments about linking or bracketing functions stem from things I've done or looked up while working on conversions. LMK if anything is unclear or changes don't make sense in terms of the checklist.
This was super helpful, thanks y'all! PR feedback should be addressed in the latest commit - let me know if not! |
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.
👍 Much appreciated!
Sorry, I totally forgot to respond to Elizabet's top level comment!
I honestly don't mind either way if people remove vs strikethrough. For myself when I do strikethroughs it's really for me to consciously say "this doesn't apply to this PR" or "I deliberately did not do this step". For more simple PRs I do just go ahead and remove steps instead - I totally agree striking through line by line is tedious 😅 I do wish GitHub had a handy keyboard shortcut for strikethrough... it looks like they don't though 😩 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6294/ |
@elastic/eui-design Going to leave this open until EOD Pacific for comments and then merge this in before I go out on leave. We can always continue tweaking the template as needed! |
The template update takes care of ensuring we list Sass removals
Preview documentation changes for this PR: https://eui.elastic.co/pr_6294/ |
Summary
yo
generator for better consistencyFeedback would be appreciated on the PR template md file (tagging multiple reviewers for that purpose). Are items well categorized? Would you know how to know if a step should or shouldn't be done with just the information provided, or is more context needed?
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
Emotion conversion checklist
withEuiTheme
need to passtrue
as the second argument to itspropUtilityForPlayground
insrc-docs/src/views/{component}/playground.js
)shouldRenderCustomStyles()
test was added and passes with parent component and any nestedchildProps
(e.g.tooltipProps
)mount()
ed snapshots in favor ofrender()
or a more specific assertion$euiSize
toeuiTheme.size.base
)yarn compile-scss
to update JSON filescalc()
tomathWithUnits
if possible (if mixing different unit types, this may not be possible)@warn
deprecation message within theglobal_styling/mixins/{component}.scss
filesrc/components/index.scss
src/amsterdam/overrides/{component}.scss
files (styles within should have been converted to the baseline Emotion styles)euiCanAnimate
gap
property to add margin between items if using flex-inline
and-block
logical properties (check inline styles as well as CSS)euiComponent
,euiComponent__child
){euiComponent}-
(case sensitive) to check for source code usage of modifier classesdata
attribute so that consumers still have something to hook into**/target, **/*.snap, **/*.storyshot
for less noise) foreui{Component}
(case sensitive) to find:euiBadge
class on a div instead of simply using theEuiBadge
component).js
docs files to TSfrom '../src'
, replace<React.Fragment>
with<>
)