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

Code refactoring of inputs tag #161

Merged
merged 10 commits into from
Oct 5, 2021
Merged

Conversation

bishwobhandari
Copy link
Contributor

@bishwobhandari bishwobhandari commented Sep 30, 2021

Implemented #161

  • added IntegerInput.js
  • added TextInput.js
  • refactored input tags with respective components.

@bishwobhandari
Copy link
Contributor Author

@shapiromatron ready for review. on the data page, when using the select input tag, there was a css issue. i will continue to work on figuring out the best way. right now its consuming more time so i just left it like it was earlier.

Copy link
Owner

@shapiromatron shapiromatron left a comment

Choose a reason for hiding this comment

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

@shapiromatron ready for review. on the data page, when using the select input tag, there was a css issue. i will continue to work on figuring out the best way. right now its consuming more time so i just left it like it was earlier.

Ok, please fix and let me know when you're ready for me to check and look around; right now I just reviewed the code.

A few changes to make our components consistent.

frontend/src/components/Main/OptionsForm/OptionsForm.js Outdated Show resolved Hide resolved
frontend/src/components/common/TextInput.js Show resolved Hide resolved
@bishwobhandari
Copy link
Contributor Author

@shapiromatron please review FloatInput.js , i just used in simple way. the only problem i see is when the input box is empty it gives warning about Nan, but when its filled it goes away. i dont think it will cause any error. we can discuss. i tried to migrate most of the html inputs to the react components. happy to receive any input.

Copy link
Owner

@shapiromatron shapiromatron left a comment

Choose a reason for hiding this comment

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

Looking good. I made a number of changes, but one of the more substantial ones was the LabelInput. One of the key attributes on the label is the for, or htmlFor in react. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label

The way it initially implemented in this PR, a new ID was created for the label and the other component, so if a user clicked a label, it wouldn't go to the desired input. This has been fixed.

I also added a TextAreaComponent in the spirit of this PR.

@shapiromatron shapiromatron changed the title Code refactoring of inputs tag. Code refactoring of inputs tag Oct 5, 2021
@shapiromatron shapiromatron merged commit 34bb1fc into main Oct 5, 2021
@shapiromatron shapiromatron deleted the code-refactoring-input-tags branch October 5, 2021 17:07
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.

None yet

2 participants