-
Notifications
You must be signed in to change notification settings - Fork 14
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
fix: make autofill work for controlled input in Firefox #2698
Conversation
🦋 Changeset detectedLatest commit: 4a53f8a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Your alpha package is ready 🎉 |
Your alpha package is ready 🎉 |
Your alpha package is ready 🎉 |
ef72e92
to
9491076
Compare
Your alpha package is ready 🎉 |
Greetings from FX team, @pudek357 👋 Thank you so much for contributing 🙇 We have got high priority ticket generated on our Kanban board so we will do our best to make your experience supreme! What's next? We will collaborate using this workflow. For you this practically means making sure DONE criteria is met and responding promptly to code review comments 😉 🙏 please, help us improve, rate your contributing experience after merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on Firefox, works as expected. Awesome how to test
in description 👍
FF on/on |
9491076
to
bbb4178
Compare
@toptal-bot run all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested manually! Looks great!
@toptal-bot run all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now we have below 4% of users who use Firefox. Do you think it's an important fix to introduce?
Hello, @denieler! Regarding #2698 (review) – I think we do need this fix, as it was reported by actual users (this affects the Screening Ops team work, for example (https://toptal-core.slack.com/archives/C01ANC5RH8B/p1650458144649619) |
@toptal-bot run all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manual tested and it works great! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the code, it looks ok 👍 Haven't tested manually 🙇♂️
🎉 Last commit is successfully deployed 🎉 Demo is available on: Your davinci-bot 🚀 |
SPT-2611
Description
There is an issue in React that breaks Firefox browser behavior for autofill/autocomplete in controlled inputs.
mui/material-ui#16943
facebook/react#15739
facebook/react#18986
The solution is taken from one of the comment:
facebook/react#18986 (comment)
How to test
open Firefox
go to https://codesandbox.io/s/withered-worker-0njelw (latest Picasso)
add some values for plain and controlled Picasso input and submit the form
check autofill for each input, you will notice that Picasso input will not have any autofill options
firefox_ntFge3HuoA_Trim.mp4
go to https://picasso.toptal.net/SPT-2611-fix-autofill-for-firefox/?path=/story/picasso-forms-final-form--final-form
edit code
Use this code to test it:
add some values for plain and controlled Picasso input and submit the form
check autofill for each input
autofill should work for both inputs
Development checks
props
in component with documentationexamples
for componentyarn test
yarn test:visual
. If not - check the documentation how to fix visual testsBreaking change
PR commands
List of available commands:
@toptal-bot run all
- Run whole pipeline@toptal-bot run build
- Check build@toptal-bot run visual
- Run visual tests@toptal-bot run deploy:documentation
- Deploy documentation@toptal-bot run package:alpha-release
- Release alpha versionPR Review Guidelines
When to approve? ✅
You are OK with merging this PR and
nit:
to your comment. (ex.nit: I'd rename this variable from makeCircle to getCircle
)When to request changes? ❌
You are not OK with merging this PR because
When to comment (neither ✅ nor ❌)
You want your comments to be addressed before merging this PR in cases like:
How to handle the comments?