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

[Cypress] Add EUI's styles to test environment #5371

Merged
merged 4 commits into from
Nov 18, 2021

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Nov 10, 2021

Summary

Opening as draft because the Cypress arguments section in cypress-testing.md is now broken.

Imports the compiled CSS theme file into Cypress's test environment for more accurate tests.

  • added scripts/cypress.js to orchestrate prod-vs-dev & skip-css logic
  • builds the CSS file by default so tests work with up-to-date styles, but allows skipping for quicker dev iteration
  • added --skip-css flag to allow avoiding CSS compilation time
  • added --dev flag to open the Cypress dev environment
  • forwards extra flags to cypress
  • updated focus_trap.spec.tsx as the styles give an empty button no size

Checklist

@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor

At a glance (although admittedly I haven't looked deeply) this approach looks solid and scripts/cypress.js LGTM! 👍 I don't see any issues with this overall approach - were there any you were anticipating?

@chandlerprall chandlerprall marked this pull request as ready for review November 17, 2021 18:35
@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor Author

chandlerprall commented Nov 17, 2021

Pushed up final changes to the cypress.js script to make everything continue working as documented, ready for review!

were there any you were anticipating?

Nope! Just didn't want to go down a rabbit hole with the script + cypress flags if there'd be pushback on the approach :)

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Working perfectly for me! 🎉

I have just one super minor comment about the theme being used, but it's absolutely not a blocker and we can move this PR forward without it or circle back to it later

if (!skipScss) {
console.log(info('Compiling SCSS'));
execSync(
`TARGET_THEME=amsterdam_dark yarn compile-scss`,
Copy link
Contributor

@cee-chen cee-chen Nov 17, 2021

Choose a reason for hiding this comment

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

Any thoughts on allowing us to specify which theme the dev wants to use as an arg? e.g. yarn test-cypress-dev --theme=dark/light or similar? I'll honestly admit I'm a garbage human being and not a huge fan of dark modes.

No worries either way, this might actually help me test more in dark modes and it's definitely not a blocker!

Copy link
Contributor Author

@chandlerprall chandlerprall Nov 18, 2021

Choose a reason for hiding this comment

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

I'll honestly admit I'm a garbage human being and not a huge fan of dark modes.

Me too! Good to know there's 2 of us 😆 (and caught up with Greg's comment, a whole 3 of us!), this was the first time I ever reached for dark mode as a default. I like your idea of choosing a theme.

Copy link
Contributor

Choose a reason for hiding this comment

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

there are dozens of us! dozens!!

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.

I'm a garbage human being and not a huge fan of dark modes

That makes two of us 🙃

I'd say merge as-is and consider theme switching later if it seems useful

@chandlerprall
Copy link
Contributor Author

I like the theme switch suggestion, as well as the one to merge this as-is. I'll create a follow-up PR with theme selection.

@chandlerprall chandlerprall merged commit 6767ec8 into elastic:main Nov 18, 2021
@chandlerprall chandlerprall deleted the making-cypress-stylish branch November 18, 2021 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants