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

Need Review of Codebase #1

Open
swashata opened this issue Apr 2, 2018 · 0 comments
Open

Need Review of Codebase #1

swashata opened this issue Apr 2, 2018 · 0 comments
Labels
help wanted Extra attention is needed

Comments

@swashata
Copy link
Member

swashata commented Apr 2, 2018

So this is my first react app. After much writing and refactoring, I have come to the conclusion as mentioned here.

I have little doubt with the current implementation, especially at point 1. Any help is appreciated.

  1. In the FontIconPicker component, the props.value is passed from outside. Which is okay. But, depending on the fact that it has to be an array if props.isMulti is true, and a string or number if false, I think it is best if I create a derived state state.value within the component. With getDerivedStateFromProps it is kept in sync, while checking for validity.

  2. Due to this nature, I am not explicitly calling this.setState. Rather I am relying on the parent to provide a handler function which would update its own state, which again would reflect on this.props.value for the FontIconPicker component. getDerivedStateFromProps would eventually set the correct state for FontIconPicker and this is what I am seeing in the dev tool too.

  3. I need to persist the state of dropdown after close. This is why, I have necessary state variables isOpen, currentPage, currentCategory, currentSearch in the FontIconPicker. All of it gets passed through props to child components (if they need it). Whenever any child needs to change it, it fires a handler of the parent FontIconPicker which keeps things in sync. If a child's state depends on any of this, then the same is managed with getDerivedStateFromProps.

I hope I am doing things right. At this stage, no styling or DOM positioning is handled, so it will look a bit weird. But the app is accessible from https://fonticonpicker.github.io/react-fonticonpicker/

The application I am building has a jQuery implementation here https://fonticonpicker.github.io/

Kindly see if any changes are required or if I am doing anything wrong.

/cc @micc83 @NiGhTTraX

@swashata swashata added the help wanted Extra attention is needed label Apr 2, 2018
swashata pushed a commit that referenced this issue Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant