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

test(banner): add darkmode screenshot test and refactor folder structure #880

Merged
merged 7 commits into from
Dec 16, 2024

Conversation

max-umain
Copy link
Collaborator

@max-umain max-umain commented Dec 9, 2024

Describe pull-request

Adds screenshot tests for banner in light and dark modes, as well as with different background colors and banner variants.

Issue Linking:

How to test

Provide detailed steps for testing, including any necessary setup.

  1. Run npm run test
  2. Visually inspect the files generated by the tests

Checklist before submission

  • I have added unit tests for my changes (if applicable)
  • All existing tests pass
  • I have updated the documentation (if applicable)
  • Not breaking production behavior
  • Behavior available in storybook with documented descriptions (if applicable)
  • npm run build-all without errors

Suggested test steps

  • Browser testing (Chrome, Safari, Firefox)
  • Keyboard operability
  • Interactive elements have labels.
  • Storybook controls
  • Design/controls/props is aligned with other components
  • Dark/light mode and variants
  • Input fields – values should be displayed properly
  • Events

Screenshots

Not applicable

Additional context

None

@max-umain max-umain mentioned this pull request Dec 9, 2024
14 tasks
Copy link
Contributor

github-actions bot commented Dec 9, 2024

Playwright test results

passed  395 passed
skipped  1 skipped

Details

stats  396 tests across 135 suites
duration  53.6 seconds
commit  d286348

Skipped tests

src/components/table/table/test/expandable-row-autocollapse/expandable-row-autocollapse.e2e.ts › tds-table-expandable-row-autoCollapse › NEEDS FIXING: expanding one row collapses the others when autoCollapse is true

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-880.d3fazya28914g3.amplifyapp.com

Copy link
Contributor

@nathalielindqvist nathalielindqvist left a comment

Choose a reason for hiding this comment

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

Neat implementation! From what I can see you have created a new folder for these test results. Sticking to the current folder structure, with 'basic', 'default', 'information' and 'error' and having the corresponding tests in their respective folders might make it easier to maintain. Looking at the already existing screenshots it looks like they have an icon in the Banner as well, which is a visual element that would need to be tested in dark and light mode as well. Maybe there is a way to build on top of what is already there? :)

@max-umain
Copy link
Collaborator Author

max-umain commented Dec 10, 2024

@nathalielindqvist
Thank you for the feedback :)

I now created a commit aee1078 where I created a file screenshotTests.ts, that I now call from the existing test files for banner. This uses the index.html files for the existing tests, so the icons in those will be present in these screenshots too.

Perhaps we could reuse the screenshotTests function (although it would need to be modified slightly to take more parameters for background color and such) in other components' tests (similar tickets in Jira). What do you think about this?

@max-umain
Copy link
Collaborator Author

Update: I removed the reusable test and created lightmode and darkmode folders in banner tests

Copy link
Contributor

@nathalielindqvist nathalielindqvist 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! 🌟

@max-umain
Copy link
Collaborator Author

One final change: @Lunkan89 and I agreed that it's unnecessary to change html files (and the images) of the existing tests if they are e.g. lightmode with white background, so I restored the html files to how they were before

Copy link
Contributor

@nathalielindqvist nathalielindqvist left a comment

Choose a reason for hiding this comment

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

Good thinking, looks nice! 🙂

@max-umain
Copy link
Collaborator Author

max-umain commented Dec 11, 2024

@nathalielindqvist @Lunkan89
Sorry, I forgot to refactor the page.goto lines to beforeEach, pushed that commit now

Copy link
Collaborator

@Lunkan89 Lunkan89 left a comment

Choose a reason for hiding this comment

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

Looks good

@max-umain max-umain force-pushed the test/banner-screenshot-tests branch from e16d2a1 to d286348 Compare December 16, 2024 09:03
@max-umain max-umain changed the title test(banner): add screenshot tests test(banner): add darkmode screenshot tests and refactor folder structure Dec 16, 2024
@max-umain max-umain changed the title test(banner): add darkmode screenshot tests and refactor folder structure test(banner): add darkmode screenshot test and refactor folder structure Dec 16, 2024
@ckrook ckrook merged commit 7783cb9 into develop Dec 16, 2024
4 checks passed
@ckrook ckrook deleted the test/banner-screenshot-tests branch December 16, 2024 12:58
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.

5 participants