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] Further enhance shouldRenderCustomStyles #6907

Merged
merged 4 commits into from
Jul 6, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jul 5, 2023

Summary

This is some final extra/optional work on top of #6896 - I didn't want to include it in the above PR as that was was big enough as it was. I recommend reviewing by commit and turning off whitespace diffs.

  • Removes skipStyles API - now uses skip: { style: true }
  • Removes skipParentTest API, now uses skip: { parentTest: true }
  • Adds a new skip API which allows for granular skipping, e.g. skip: { css: true }
  • Updates all tests using the old APIs (particularly ones that were skipping styles due to ...rest spread - add tests that actually checks that consumer styles are correctly applied)

QA

General checklist

N/A, internal dev/tech debt only

cee-chen added 4 commits July 5, 2023 12:54
- use an obj API for skips

- programmatically generate props objects as well as test titles based on skips
- to components that spread style to a different DOM node than `className`

- we can now add these tests thanks to the new `targetSelector` API and granular skips
@cee-chen cee-chen changed the title [Emotion] Further improve shouldRenderCustomStyles [Emotion] Further enhance shouldRenderCustomStyles Jul 5, 2023
@cee-chen cee-chen requested a review from a team July 5, 2023 20:13
@cee-chen cee-chen added skip-changelog testing Issues or PRs that only affect tests - will not need changelog entries tech debt emotion labels Jul 5, 2023
@cee-chen cee-chen marked this pull request as ready for review July 5, 2023 20:15
Comment on lines -70 to +99
const testCases = options.skipStyles
? 'classNames and css'
: 'classNames, css, and styles';
const testProps = options.skipStyles
? { className: customStyles.className, css: customStyles.css }
: customStyles;
// Account for any skipped props
const testProps = keysOf(customStyles).reduce((map, key) => {
return options.skip?.[key] ? map : { ...map, [key]: customStyles[key] };
}, {} as Partial<typeof customStyles>);

// Generate a grammatically excellent list of props being tested
let propsToTest = '';
const propsToTestArr = Object.keys(testProps);
switch (propsToTestArr.length) {
case 1:
propsToTest = propsToTestArr[0];
break;
case 2:
propsToTest = `${propsToTestArr[0]} and ${propsToTestArr[1]}`;
break;
// You'll pry the oxford comma from my cold dead hands
default:
propsToTest = `${propsToTestArr
.slice(0, -1)
.join(', ')}, and ${propsToTestArr.slice(-1)}`;
break;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screencap of what this should output put grammatically in test titles, based on number of props being tested:

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6907/

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 Refactoring to the skip API is a good way to corral the various options. Now users can pick which facet(s) they want to test.

@cee-chen
Copy link
Contributor Author

cee-chen commented Jul 6, 2023

🎉 Thanks again Trevor!

@cee-chen cee-chen merged commit 938fb90 into elastic:main Jul 6, 2023
@cee-chen cee-chen deleted the fix-css-merging-3 branch July 6, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emotion skip-changelog tech debt testing Issues or PRs that only affect tests - will not need changelog entries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants