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

Modal tests #2377

Merged
merged 4 commits into from
Aug 14, 2023
Merged

Modal tests #2377

merged 4 commits into from
Aug 14, 2023

Conversation

ofhope
Copy link
Contributor

@ofhope ofhope commented Aug 13, 2023

Fixes #issue-number

This PR adds storybook documentation and unit tests to the modal component.

Screenshot 2023-08-11 at 6 06 11 am

It also looks like a bable upgrade along the way broke storybook hot module reloading.
I didn't spend a lot of time investigating the cause but did find a solution to monkey patching the issue. HMR was still working in storybook but un-viewable without this patch.

Screenshot 2023-08-11 at 6 05 10 am

Changes:

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@lindapaiste
Copy link
Collaborator

It also looks like a bable upgrade along the way broke storybook hot module reloading. I didn't spend a lot of time investigating the cause but did find a solution to monkey patching the issue. HMR was still working in storybook but un-viewable without this patch.

THANK YOU! I've been seeing that error but hadn't had a chance to look into it yet.

Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

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

Looks great! I love the tests for the inside and outside click behavior and I so grateful for the window.$RefreshReg$ fix.

@lindapaiste
Copy link
Collaborator

It's a bummer that it doesn't render with the proper styling. There's two issues there, one of which I only just now realized! Maybe you can fix it in a separate PR.

  1. The modal needs to have a parent with class="light" (or "dark" or "contrast"). This will be fixed by Add theme switching to Storybook #2326, but we can also put in a quick fix until that gets merged since it might be a while. We can add a <div className="light"> to the decorators in preview.js.
  2. The compiled main.css file that we are importing in preview.js is not up to date. I don't know what that file is for and probably it should be deleted. We want to import the uncompiled file import '../client/styles/main.scss' as that will be current will all changes. But in order to do that we also need a Storybook addon to process it: https://storybook.js.org/addons/storybook-addon-sass-postcss

@lindapaiste lindapaiste merged commit 62003a9 into processing:develop Aug 14, 2023
1 check passed
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.

2 participants