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.

Sign up for GitHub

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

Jump to bottom

Test(e2e): Modal Visual Tests #DS-1247 #1547

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Test(e2e): Modal Visual Tests #DS-1247 #1547

merged 1 commit into from
Jul 23, 2024

Conversation

pavelklibani
Copy link
Contributor

Description

Additional context

Issue reference

Udělat vizuální test otevřeného Modalu

@pavelklibani pavelklibani added the run-visual-tests Runs visual regression testing on this PR label Jul 17, 2024
@pavelklibani pavelklibani self-assigned this Jul 17, 2024
@github-actions github-actions bot added the feature New feature or request label Jul 17, 2024
@netlify Netlify
Copy link

netlify bot commented Jul 17, 2024 •
edited
Loading

✅ Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit de7a4cf
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/669f7c5ad53060000833cd49
😎 Deploy Preview https://deploy-preview-1547--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		 1 paths audited
Performance: 95 (🔴 down 1 from production)
Accessibility: 93 (no change from production)
Best Practices: 100 (no change from production)
SEO: 82 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify Netlify
Copy link

netlify bot commented Jul 17, 2024 •
edited
Loading

✅ Deploy Preview for spirit-design-system-storybook ready!

Name Link
🔨 Latest commit de7a4cf
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/669f7c5ae584250008e2f6ae
😎 Deploy Preview https://deploy-preview-1547--spirit-design-system-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pavelklibani pavelklibani force-pushed the feat/ds-1247-pened-modal-visual-test branch from 2807346 to 807b3a3 Compare July 17, 2024 19:01
@coveralls
Copy link

coveralls commented Jul 17, 2024 •
edited
Loading

Coverage Status

coverage: 78.187%. remained the same
when pulling de7a4cf on feat/ds-1247-pened-modal-visual-test
into 80f8f2b on main.

@github-actions GitHub Actions
Copy link
Contributor

Here is the URL of the uploaded artifact: https://github.com/lmc-eu/spirit-design-system/actions/runs/9979951946/artifacts/1712189591

@pavelklibani pavelklibani force-pushed the feat/ds-1247-pened-modal-visual-test branch from 807b3a3 to 3b68b6c Compare July 18, 2024 08:08
@github-actions GitHub Actions
Copy link
Contributor

Here is the URL of the uploaded artifact: https://github.com/lmc-eu/spirit-design-system/actions/runs/9987863537/artifacts/1714113484

crishpeen
crishpeen reviewed Jul 18, 2024
View reviewed changes
Copy link
Member

@crishpeen crishpeen left a comment

Choose a reason for hiding this comment

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

Great work, just some ideas :)

packages/web/src/scss/components/Modal/index.html Outdated Show resolved Hide resolved
tests/e2e/demo-components-compare.spec.ts-snapshots/fileuploader-chromium-linux.png Outdated Show resolved Hide resolved
tests/e2e/demo-homepages.spec.ts-snapshots/web-chromium-linux.png Outdated Show resolved Hide resolved
tests/e2e/demo-homepages.spec.ts-snapshots/web-react-chromium-linux.png Outdated Show resolved Hide resolved
tests/e2e/demo-modal-compare.spec.ts Outdated Show resolved Hide resolved
tests/e2e/demo-modal-compare.spec.ts Outdated Show resolved Hide resolved
tests/e2e/demo-modal-compare.spec.ts Outdated Show resolved Hide resolved
tests/e2e/demo-modal-compare.spec.ts Outdated Show resolved Hide resolved
tests/e2e/demo-modal-compare.spec.ts Outdated Show resolved Hide resolved
packages/web-react/src/components/Modal/demo/ModalDefault.tsx Show resolved Hide resolved
@pavelklibani pavelklibani force-pushed the feat/ds-1247-pened-modal-visual-test branch 3 times, most recently from 93a4b08 to 9cff822 Compare July 18, 2024 13:25
@pavelklibani pavelklibani force-pushed the feat/ds-1247-pened-modal-visual-test branch from 9cff822 to 4be5cfe Compare July 19, 2024 12:31
@literat
Copy link
Collaborator

literat commented Jul 19, 2024

Maybe note to the future: Do we need to test those Modals visually? That is a lot of images. So wouldn't it be appropriate to test it by integration test style, e.g. click on a button, modal opens, find in DOM some nodes, click on a backdrop, click on the close button, fire ESC event, etc...

literat
literat approved these changes Jul 19, 2024
View reviewed changes
Copy link
Collaborator

@literat literat left a comment

Choose a reason for hiding this comment

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

I love the way you have refactored the test and created new helpers for it. Good Job 👍

There is also an open question whether we should do these tests in a visual way. I can imagine that repeating this for Tooltip as well as for Dropdow will lead us to so many images.

@pavelklibani
Copy link
Contributor Author

Maybe note to the future: Do we need to test those Modals visually? That is a lot of images. So wouldn't it be appropriate to test it by integration test style, e.g. click on a button, modal opens, find in DOM some nodes, click on a backdrop, click on the close button, fire ESC event, etc...

We agreed to make tests for all modals in the office while you were in Poland. So I will leave it like this for now. We can remove some in the future.

@pavelklibani pavelklibani force-pushed the feat/ds-1247-pened-modal-visual-test branch from 4be5cfe to de7a4cf Compare July 23, 2024 09:48
@pavelklibani
Copy link
Contributor Author

pavelklibani commented Jul 23, 2024 •
edited
Loading

conflicts solved and rebased to main

@pavelklibani pavelklibani merged commit 305b252 into main Jul 23, 2024
27 checks passed
@pavelklibani pavelklibani deleted the feat/ds-1247-pened-modal-visual-test branch July 23, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crishpeen crishpeen crishpeen left review comments

@literat literat literat approved these changes

@curdaj curdaj curdaj approved these changes

@adamkudrna adamkudrna Awaiting requested review from adamkudrna adamkudrna is a code owner

Assignees

@pavelklibani pavelklibani

Labels
feature New feature or request run-visual-tests Runs visual regression testing on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pavelklibani @coveralls @literat @crishpeen @curdaj