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

[No QA] Create FORMS.md #7958

Merged
merged 6 commits into from
Mar 2, 2022
Merged

[No QA] Create FORMS.md #7958

merged 6 commits into from
Mar 2, 2022

Conversation

luacmartins
Copy link
Contributor

@luacmartins luacmartins commented Mar 1, 2022 •
edited by flodnv
Loading

Details

Creates a README with guidelines for Form and Form input based on the solution implemented by the design doc.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/194613

Tests

  1. Open FORMS.md and verify that you can read the file.
  • Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

  • I made sure to pull main before submitting my PR for review
  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I clearly indicated the environment tests should be run in (Staging vs Production)
  • I wrote testing steps that cover success & fail scenarios (if applicable)
  • I ran the tests & they passed on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified there are no console errors related to changes in this PR
  • I followed proper code patterns (see Reviewing the code)
    • I added comments when the code was not self explanatory
    • I put all copy / text shown in the product in all src/languages/* files (if applicable)
    • I followed proper naming convention for platform-specific files (if applicable)
    • I followed style guidelines (in Styling.md) for all style edits I made
  • I followed the guidelines as stated in the Review Guidelines

PR Reviewer Checklist

  • I verified the Author pulled main before submitting the PR
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I verified tests pass on all platforms & I tested again on all platforms
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines

QA Steps

None. Adding a readme file.

  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@luacmartins luacmartins self-assigned this Mar 1, 2022
@luacmartins luacmartins marked this pull request as ready for review March 1, 2022 21:31
@luacmartins luacmartins requested a review from a team as a code owner March 1, 2022 21:31
@MelvinBot MelvinBot requested review from flodnv and removed request for a team March 1, 2022 21:31
flodnv
flodnv requested changes Mar 1, 2022
View reviewed changes
FORMS.md Show resolved Hide resolved
FORMS.md Outdated Show resolved Hide resolved
FORMS.md

```
<TextInput
name="fname"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the significance of "fname" here? 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the attribute used by the browser to autofill the input. In this case first name. There's a link with references to each attribute available. I tweak this description slightly to hopefully make it clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, can we name it firstName then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I don't think so since the name attribute used by the browser for given-name (first name) is fname?

Screen Shot 2022-03-02 at 1 01 25 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! Oh! Wow! Thanks for the clarification, after visiting the link this definitely makes sense 👍

FORMS.md Outdated Show resolved Hide resolved
FORMS.md Outdated Show resolved Hide resolved
FORMS.md Outdated Show resolved Hide resolved
FORMS.md Show resolved Hide resolved
FORMS.md Outdated Show resolved Hide resolved
FORMS.md Show resolved Hide resolved
FORMS.md Show resolved Hide resolved
@luacmartins
Copy link
Contributor Author

Updated! Thanks for the thorough review!

@luacmartins luacmartins requested a review from flodnv March 2, 2022 00:16
flodnv
flodnv reviewed Mar 2, 2022
View reviewed changes
Copy link
Contributor

@flodnv flodnv left a comment

Choose a reason for hiding this comment

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

Neat, thanks! Just one last comment left.

This doc is great. However, as with all docs, it will get outdated with time. Should we perhaps add some sort of reminder that when modifying the Form.js file(s), this doc should be updated if necessary? This can of course be done in a separate issue & PR.

@luacmartins
Copy link
Contributor Author

Great idea! I created an issue for that - https://github.com/Expensify/Expensify/issues/199835. Thanks!

@flodnv flodnv merged commit 9d75df5 into main Mar 2, 2022
@flodnv flodnv deleted the cmartins-formReadme branch March 2, 2022 20:23
@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2022

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Mar 10, 2022
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @flodnv in version: 1.1.42-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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

@flodnv flodnv flodnv approved these changes

Assignees

@luacmartins luacmartins

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@luacmartins @OSBotify @flodnv