-
Notifications
You must be signed in to change notification settings - Fork 34
add forms issues #1338, #1342, #1344 #1387
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
Conversation
Generated by 🚫 Danger |
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.
First of all, thank you for the contribution! 👍
I've left comments and first we need to fix simple and obvious issues. After that we will make it work!
Note that
- all messages/labels/placeholders should have localization that is taken from
props
- where possible, keep the same style as everywhere else
- don't add non-related changes (like formatting)
- elements' IDs should have meaningful names (it's important for writing integration tests and also that's why this PR broke the existing tests)
For future: every issue should be fixed by a separate PR as it's easier to review and deal with. Let's leave this PR as-is but next time, please, create separate branches and PRs for the issues.
P.S. ping me once this will be ready for another round of review. |
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.
I quickly looked on new changes, thank you! See my comments.
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.
Thank you!
I posted more comments and improvements.
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.
More comments after I looked on a prototype deeply.
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.
Now it looks much better! Thank you! Just a couple comments and later we should split commits and remove whitespace related changes.
Thank you for contribution! 🥇 I've added you to https://github.com/php-coder/mystamps/wiki/team-members |
No description provided.