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] Fix/regression test more css props #6248

Merged
merged 10 commits into from
Sep 20, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Sep 20, 2022

Summary

Follow up to #6239, closes #6152

This PR:

  • Adds tests/fixes more components that have already been converted to Emotion
  • Updates the shouldRenderCustomStyles API to use a more future-proof options obj that allows the following:
    • Skipping the styles assertion (since some components explicitly disallow custom styles)
    • Skipping the parent component test (since some tests have multiple setups/assertions for child props)
  • Adds regression tests for a few more tests that were super quick / did not require any source code changes
  • ⚠️ Does not fix/regression test all remaining components that have childProps - the changelist was getting large, and the tooltip props are slightly tricky, so I'm going to split it into (yet another) follow-up PR

Checklist

@cee-chen cee-chen changed the title [Emotion] Fix/regression test more css props not being correctly merged [Emotion] Fix/regression test more css props Sep 20, 2022
@cee-chen cee-chen marked this pull request as ready for review September 20, 2022 00:49
@cee-chen cee-chen mentioned this pull request Sep 20, 2022
21 tasks
@kibanamachine
Copy link

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

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Does not fix/regression test all remaining components that have childProps - the changelist was getting large, and the tooltip props are slightly tricky, so I'm going to split it into (yet another) follow-up PR

Sounds good!

Comment on lines +58 to +62
options: {
childProps?: string[];
skipStyles?: boolean;
skipParentTest?: boolean;
} = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this API 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Testing] shouldRenderCustomStyles account for more configurations
3 participants