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

Doc/casey j may/text fields #199

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

CaseyJMay
Copy link
Collaborator

Still have some question parts and parts that aren't commented because I'm not quite sure how they work, but wanted to put a pull request to contain feedback and monitor progress.

@18chowdhary 18chowdhary changed the base branch from master to dev April 19, 2022 23:17
Copy link
Collaborator

@18chowdhary 18chowdhary left a comment

Choose a reason for hiding this comment

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

Other changes to make but Github wouldn't let me leave comments on the files:

  • Add a header comment to the top of the LargeTextFiedl file describing what the component is - refer to the other components for examples. In the header comment, make sure to explain what each of the props to this component are. Can be very brief in a bullet list.
  • Make sure all single line comments are preceded with // instead of /*.
  • Follow a similar process for OpenTextField and OpenTextFieldWithSelection.

@@ -53,7 +53,7 @@ const LargeTextField = (props) => {
}
}


/* Calls when text is updated, evaluates the validity of text based on TextFieldValidation component */
const onTextChange = (text) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would definitely add more comments to the body of this function explaining how the validation works.

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

Successfully merging this pull request may close these issues.

2 participants