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

Target size feature #248

Merged
merged 55 commits into from
Mar 31, 2022
Merged

Target size feature #248

merged 55 commits into from
Mar 31, 2022

Conversation

gerardo-rodriguez
Copy link
Member

@gerardo-rodriguez gerardo-rodriguez commented Sep 10, 2021 •
edited
Loading

Closes #72

@gerardo-rodriguez gerardo-rodriguez self-assigned this Sep 10, 2021
@calebeby
Copy link
Member

@gerardo-rodriguez It looks like the unconventional newline character doesn't get displayed in CI on GitHub. Starting to think maybe we should remove it

@calebeby
Copy link
Member

@gerardo-rodriguez Do you anticipate having time to finish this up? If not, I can.

calebeby
calebeby reviewed Oct 29, 2021
View reviewed changes
tests/user/click.test.ts Show resolved Hide resolved
src/ansi-colors-browser.ts Outdated Show resolved Hide resolved
@gerardo-rodriguez
Remove unconventional new line character
3db4b77
@gerardo-rodriguez
Merge branch 'feature/touch-target-size' of https://github.com/cloudf…
53f697e 
…our/pleasantest into feature/touch-target-size

* 'feature/touch-target-size' of https://github.com/cloudfour/pleasantest:
  Update tests/user/click.test.ts
  Update tests/user/click.test.ts
  Update src/user-util/index.ts
@gerardo-rodriguez
Update snapshots
cb85528
@gerardo-rodriguez
Copy link
Member Author

I'm not sure if I was supposed to update the snapshots. 🤔

@gerardo-rodriguez
Copy link
Member Author

@calebeby If you have any time to team up on this to try and knock it out in the coming week, let me know and we can schedule some time. 😉

@calebeby
Copy link
Member

I should have time on Monday, I don't have classes! We can work out a time then

@calebeby calebeby mentioned this pull request Nov 4, 2021
Merged
@calebeby
Copy link
Member

calebeby commented Nov 8, 2021

Marking this as ready for review, but also once @gerardo-rodriguez is back he will need to review the docs changes I made as well.

@gerardo-rodriguez
Copy link
Member Author

@calebeby I refactored the src/user-util/index.ts logic. The inline snapshots are now failing but I'm not sure what to do. I think they need to get updated but I don't know how to update them within CI.

When you have a moment, can you take a look and let me know how I should handle this? Thanks! 👍

@calebeby
Copy link
Member

calebeby commented Nov 16, 2021 •
edited
Loading

@gerardo-rodriguez you can make the snapshots pass in CI by forcing the elements to have a fixed size (like adding width/height etc) so that differences in font rendering or browser defaults (maybe?) won't change the element's size between local + CI

Like I did in this commit: 2e8101a

@gerardo-rodriguez
Copy link
Member Author

TODO: Add changeset and decide whether to leave this open instead of merging it right away since it is a breaking change

It may make sense to do a minor release with accessibility snapshots, followed by a major change with the CSS changes I am working on as well as this PR.

@calebeby I have addressed all PR feedback, but these items from you are still outstanding. I'm not sure what is actionable for me to do.

calebeby
calebeby reviewed Nov 19, 2021
View reviewed changes
src/user-util/index.ts Outdated Show resolved Hide resolved
@calebeby
Copy link
Member

@gerardo-rodriguez I finished refactoring! I think this PR is all good to go, except that I want to hold off on merging until after I do a minor release, because this is a breaking change.

@calebeby calebeby merged commit abe22a6 into main Mar 31, 2022
@calebeby calebeby deleted the feature/touch-target-size branch March 31, 2022 20:57
@github-actions github-actions bot mentioned this pull request Mar 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calebeby calebeby calebeby approved these changes

@spaceninja spaceninja Awaiting requested review from spaceninja

@Paul-Hebert Paul-Hebert Awaiting requested review from Paul-Hebert

Assignees

@gerardo-rodriguez gerardo-rodriguez

Labels
major change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Target size for .click()
3 participants
@gerardo-rodriguez @calebeby @Paul-Hebert