close
Skip to content
This repository was archived by the owner on Mar 1, 2024. It is now read-only.

fix camel-case capitalization of minLength and maxLength in FormInput component#416

Closed
ubergesundheit wants to merge 1 commit into
tabler:masterfrom
ubergesundheit:min-max-length-form-input-fix
Closed

fix camel-case capitalization of minLength and maxLength in FormInput component#416
ubergesundheit wants to merge 1 commit into
tabler:masterfrom
ubergesundheit:min-max-length-form-input-fix

Conversation

@ubergesundheit
Copy link
Copy Markdown

I noticed my tests and development versions printed out this warning when using Form.Input components:

Warning: Invalid DOM property `maxlength`. Did you mean `maxLength`?
          in input (created by Form.Input)
          in Form.Input (created by DropdownMenu)
          in div (created by Form.InputGroup)
         ....

This pull request fixes this issue by renaming minlength to minLength and maxlength to maxLength in src/components/Form/FormInput.react.js

I guess this is somewhat a breaking change but it makes tabler-react more correct against the official standards.

Cheers!

Copy link
Copy Markdown
Contributor

@jonthomp jonthomp left a comment

Choose a reason for hiding this comment

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

@ubergesundheit I was tempted to just release this as minor bump rather than breaking change but sticking to the rules it should be breaking as you suggest 🤷‍♂️

What do you think about keeping all 4 options so we avoid breaking?

@ubergesundheit
Copy link
Copy Markdown
Author

Fine with me, as long as the warning is resolved. I'll see if keeping both options triggers the warning or not.

@ubergesundheit ubergesundheit force-pushed the min-max-length-form-input-fix branch from 83c1dda to 0a9f44c Compare March 11, 2019 16:02
@ubergesundheit
Copy link
Copy Markdown
Author

@jonthomp I tried your suggestion adding the correct properties instead of replacing, but this triggers the invalid DOM property warning.

I am fine with this being merged into a major release.

@PetrVasilev
Copy link
Copy Markdown

I'm starting to get annoyed by this error. When can you fix it?

@jonthomp
Copy link
Copy Markdown
Contributor

Closing this and merging #440 instead as includes an additional fix

@jonthomp jonthomp closed this May 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants