Skip to content

Comments

Adding a new feature to add color value via typing. Unnecessary whitespace removed.#1

Open
matysanchez wants to merge 2 commits intoLeaVerou:gh-pagesfrom
matysanchez:gh-pages
Open

Adding a new feature to add color value via typing. Unnecessary whitespace removed.#1
matysanchez wants to merge 2 commits intoLeaVerou:gh-pagesfrom
matysanchez:gh-pages

Conversation

@matysanchez
Copy link

Now the user can choose the color with the color picker or typing it.

@LeaVerou
Copy link
Owner

LeaVerou commented Feb 4, 2014

Hi there,
Thanks for the PR (how did you discover this project?!) but the code could use some work. For example:

  • onchange is a bad event to use for text inputs, as it fires after the user tabs away from the input. oninput is a better one.
  • If the color control is not supported, you get two textboxes
  • It uses inline HTML event handlers which are a bad practice
  • maxlength="7" is a bad idea for colors, as there are longer notations that should be supported (e.g. rgb() or hsl())
  • You're using a string as a flag and who is not a descriptive variable name

@matysanchez
Copy link
Author

sin ttulo

I will work on the points you mentioned and I will add a commit to this PR.

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