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

feat: implement CodeInput component #1524

Merged
merged 9 commits into from
May 8, 2020

Conversation

ghost
Copy link

@ghost ghost commented May 4, 2020

fix: #1488

feat: implement codeInput component first revision

@commit-lint
Copy link

commit-lint bot commented May 4, 2020

Features

  • implement codeInput component first revision (ef3a384)

Bug Fixes

  • component behavior update, most of the logic was changed based on comments from first revision (db22a1d)
  • codeInput component, now we are controlling most of the input focus logic using states (2e51410)
  • set length prop as optional (edceebd)
  • quick fixes to reduce code and make it cleaner (7e9e4e7)
  • ref management update, so we don't ask to user for a mandatory ref (952f56a)
  • ref usage with useImperativeHandle (7050486)
  • lint fix to prioritize DELETE_KEY import over prop-types (366e0df)

Contributors

@blakmetall, @LeandroTorresSicilia

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@ghost ghost requested a review from LeandroTorresSicilia May 4, 2020 05:52
@TahimiLeonBravo TahimiLeonBravo self-requested a review May 4, 2020 16:21
@yvmunayev yvmunayev self-requested a review May 5, 2020 10:15
Copy link
Contributor

@yvmunayev yvmunayev left a comment

Choose a reason for hiding this comment

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

I think that I should rethink your life cycle because it is very complex and makes you work harder. I think the first step should be to normalize "value" in an array and that array is core of your component. This is going to give you a few validations and checks now.

Try to get the functions out for the helpers and optimize using hook.

src/components/CodeInput/readme.md Outdated Show resolved Hide resolved
src/components/CodeInput/readme.md Outdated Show resolved Hide resolved
src/components/CodeInput/readme.md Outdated Show resolved Hide resolved
src/components/CodeInput/readme.md Outdated Show resolved Hide resolved
src/components/CodeInput/readme.md Outdated Show resolved Hide resolved
src/components/CodeInput/index.d.ts Outdated Show resolved Hide resolved
src/components/CodeInput/index.js Outdated Show resolved Hide resolved
src/components/CodeInput/index.d.ts Outdated Show resolved Hide resolved
src/components/CodeInput/index.js Outdated Show resolved Hide resolved
src/components/CodeInput/inputItems.js Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented May 5, 2020

I think that I should rethink your life cycle because it is very complex and makes you work harder. I think the first step should be to normalize "value" in an array and that array is core of your component. This is going to give you a few validations and checks now.

Try to get the functions out for the helpers and optimize using hook.

sure, thank you for your comments, applying all changes right away.

…n comments from first revision

* added useReduxForm hook inside the hooks export list
* hooks list arranged alphabetically
@ghost ghost requested a review from yvmunayev May 6, 2020 08:51
@ghost
Copy link
Author

ghost commented May 6, 2020

Changes ready for a second revision...

About the length prop you mentioned, it is used to establish the codeLength to be used for the component...

This is the file with the props definitions I have

https://docs.google.com/document/d/1PAFwTznCcUdz3_Vp9NJ5o0EFm3N4qcRY2GWQ1IXGl7Q/edit


I changed the name from length to codeLength to be more explicit, if you think its best to be called different please let me know.

Copy link
Contributor

@yvmunayev yvmunayev left a comment

Choose a reason for hiding this comment

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

I believe that the life cycle of this component should be this:

a variable values ​​that is an array that is calculated based on the prop value and lenght, with that array you have the data to paint in complete component. assume this "1234" = [1, 2, 3, 4], "1 34" = [1, "", 3, 4], "12" = [1, 2, "", ""] = and " "= [" "," "," "," "] put this in a custom hook and use useMemo

a focusedIndex state that controls which input the focus is on

handleChange (newValue, index) converts the array to string and calls onChange

make a custom hook that has all the logic of keyPress

To optimize the repainting of nodes you can use a context where the fucusedIndex state and the handleChange are passed.

in the render you make a values.map for InputItem

the InputItem component is simple

props value, index and get from context fucusedIndex and handleChange

a useEffect to put focus

use keyDownPress to only enter numbers

and the onChange of the input calls handleChange (value, index)

I think you can delete the InputItems, all that logic can be in index.js

try to optimize everything you can using useMeme and useCallback.

@ghost
Copy link
Author

ghost commented May 7, 2020

I believe that the life cycle of this component should be this:

a variable values ​​that is an array that is calculated based on the prop value and lenght, with that array you have the data to paint in complete component. assume this "1234" = [1, 2, 3, 4], "1 34" = [1, "", 3, 4], "12" = [1, 2, "", ""] = and " "= [" "," "," "," "] put this in a custom hook and use useMemo

a focusedIndex state that controls which input the focus is on

handleChange (newValue, index) converts the array to string and calls onChange

make a custom hook that has all the logic of keyPress

To optimize the repainting of nodes you can use a context where the fucusedIndex state and the handleChange are passed.

in the render you make a values.map for InputItem

the InputItem component is simple

props value, index and get from context fucusedIndex and handleChange

a useEffect to put focus

use keyDownPress to only enter numbers

and the onChange of the input calls handleChange (value, index)

I think you can delete the InputItems, all that logic can be in index.js

try to optimize everything you can using useMeme and useCallback.

thank you for all your comments, they totally changed the way I was seeing the component...

@TahimiLeonBravo TahimiLeonBravo self-requested a review May 7, 2020 17:12
@ghost ghost changed the title feat: implement codeInput component first revision feat: implement codeInput component May 8, 2020
@ghost ghost changed the title feat: implement codeInput component feat: implement CodeInput component May 8, 2020
@ghost ghost requested a review from LeandroTorresSicilia May 8, 2020 01:27
@LeandroTorresSicilia LeandroTorresSicilia merged commit 3e953fd into master May 8, 2020
@LeandroTorresSicilia LeandroTorresSicilia deleted the feat-implement-codeinput-component branch May 8, 2020 05:24
} = props;

const inputs = value.map((inputValue, index) => {
const inputIndex = index;

Choose a reason for hiding this comment

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

const inputIndex = `code-input-${index}`;


return (
<StyledInput
key={index}

Choose a reason for hiding this comment

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

remove key={index}

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.

feat: implement CodeInput component
4 participants