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(popper): additional popper and popover e2e tests #141

Merged

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Jun 4, 2020

This adds some tests to check the arrow positions for the Popover and different reference types for the Popper. When merged, issue #83 can be closed.

Along the way I also fixed a few typos and a bug (see commit 209e5be and this issue floating-ui/react-popper#354)

@HendrikThePendric HendrikThePendric requested a review from a team as a code owner June 4, 2020 10:52
@HendrikThePendric HendrikThePendric self-assigned this Jun 4, 2020
@HendrikThePendric
Copy link
Contributor Author

BTW: Can I just keep these commit as the are? Or should I squash them before merging?

@cypress
Copy link

cypress bot commented Jun 4, 2020



Test summary

481 0 0 0


Run details

Project ui
Status Passed
Commit a1a29e9
Started Jun 9, 2020 11:43 AM
Ended Jun 9, 2020 11:53 AM
Duration 10:11 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 80

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Given('a Popover is rendered with placement left', () => {
cy.visitStory('Popover', 'Placement Left')
})
Given('a Popover with position left is shited into view', () => {
Copy link
Member

@ismay ismay Jun 8, 2020

Choose a reason for hiding this comment

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

typo, shifted

Copy link
Contributor

Choose a reason for hiding this comment

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

😆

})

Then('the Arrow is horizontally aligned with the Popper', () => {
cy.getPositionsBySelectors(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should resolve this first: #106

@ismay
Copy link
Member

ismay commented Jun 8, 2020

BTW: Can I just keep these commit as the are? Or should I squash them before merging?

I would use the test prefix for all changes related to the tests. The refactor commit type shows up in the changelog for instance.

I you ask me, I'd say squash to a commit that changes the popper test, a commit that does the popover tests, and the one with the sharedPropType fix. Unless they're all somehow related.

@ismay
Copy link
Member

ismay commented Jun 8, 2020

Looks good to me. I personally think it'd be nice to resolve #106 if we can, before merging this. That way we won't be introducing more tests that need to be refactored later. Other than that and the small typo this looks good to merge to me.

Copy link
Contributor

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

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

What @ismay said. I think the commit messages could be squashed

@HendrikThePendric HendrikThePendric force-pushed the add-additional-popper-and-popover-e2e-tests branch from d109f03 to e7585d9 Compare June 9, 2020 11:29
@HendrikThePendric
Copy link
Contributor Author

I've squashed all my commits, fixed the typo and have implemented the position assertions as discussed in #106 (and a little bit on Slack). So I think this is now ready for a re-review.

@HendrikThePendric HendrikThePendric force-pushed the add-additional-popper-and-popover-e2e-tests branch from 6bda491 to 0356ec0 Compare June 9, 2020 11:33
Commit history (before squashing)
- fix(popover): add position absolute to arrow to prevent misplacement (See floating-ui/react-popper#354)
- test(popover): add e2e tests and stories for arrow positions
- chore(popover): fix typo in feature file and test
- refactor(popper): replace e2e story decorator by helper component (This prepares the story for the new e2e test which are not going to be compatible with the decorator logic)
- fix(popper): use correct sharedPropType name
- test(popper): prepare e2e stories about different types of references
- test(popper): add e2e test and stories for different types of references
- chore(popper): stop using `getPositionsBySelectors` helper in new tests
- chore(popover): stop using `getPositionsBySelectors` helper in new tests
- chore(popover): fix typo in feature step definition
@HendrikThePendric HendrikThePendric force-pushed the add-additional-popper-and-popover-e2e-tests branch from e617ec4 to a1a29e9 Compare June 9, 2020 11:40
@HendrikThePendric HendrikThePendric merged commit c1023be into master Jun 9, 2020
@HendrikThePendric HendrikThePendric deleted the add-additional-popper-and-popover-e2e-tests branch June 9, 2020 11:55
@dhis2-bot
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

None yet

4 participants