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.

Sign up for GitHub

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

Jump to bottom

[CLOSED] Use upper/lowercase colors in Inline Color Editor #8558

Open
core-ai-bot opened this issue Aug 30, 2021 · 26 comments
Open

[CLOSED] Use upper/lowercase colors in Inline Color Editor #8558

core-ai-bot opened this issue Aug 30, 2021 · 26 comments

Comments

@core-ai-bot
Copy link
Member

Issue by le717
Sunday Oct 19, 2014 at 00:34 GMT
Originally opened as adobe/brackets#9596

For #9519.

Features

  • Preserves case between value conversions and slider changes
  • Related code updated to be case-insensitive
  • New tests for conversion, with all existing tests passing
  • Also made some minor code style fixes

This code is rather confusing (as already documented), so it was a bit of a challenge to get this written up. I once spent 30 digging around in various places for a one-line fix.

Now for the issues present here:

  • This requires an update to TinyColor v1.1.0+, included with this PR. The creator refactored the library quite a bit since the v0.9.10 version currently used. TinyColor also unconditionally converts input to lowercase, which would make us have to keep track if the input is uppercase or not (which, in line with the previously documented issues, would be very hard because of the changing types). I have worked with Brian on the aforementioned patch to store the original input, which in turn makes this patch much easier to work out.

le717 included the following code: https://github.com/adobe/brackets/pull/9596/commits
@core-ai-bot
Copy link
Member Author

Comment by Mark-Simulacrum
Sunday Dec 21, 2014 at 16:19 GMT

The "U" toggle button doesn't seem appropriate for a button of it's type. Perhaps it should be separated from the RGBa, Hex, and HSLa button-container.@larz0 will probably have a better idea, though.

At the very least, I would perhaps make the button have more text than "U" as that seems rather odd. Another improvement would be having the button change state depending on the current state (U/L, for example (Uppercase/Lowercase)). Again,@larz0 will probably have better design suggestions.

@core-ai-bot
Copy link
Member Author

Comment by Mark-Simulacrum
Sunday Dec 21, 2014 at 16:55 GMT

I also found what seems like a bug. Whenever I:

  1. Edit a lowercase color (#ecc).
  2. Change the color type to HSLa. (hsl(0, 50%, 87%))
  3. Hit "U" to go uppercase. (HSL(0, 50%, 87%))
  4. Change opacity to another value.
  5. Color is now hsla(0, 50%, 87%, 0.95), not the expected HSLA(0, 50%, 87%, 0.95).

Any change to the color (opacity, color wheel, actual color) also seems to reset the uppercase to lowercase.

@core-ai-bot
Copy link
Member Author

Comment by larz0
Sunday Dec 21, 2014 at 17:10 GMT

This should be a property in brackets.json so that the future inline gradient editor can stick to the preferred case preference; brackets.json will have a UI eventually.

@core-ai-bot
Copy link
Member Author

Comment by le717
Monday Dec 22, 2014 at 02:03 GMT

@larz0 I haven't touched the preferences system before, that will be new to me. IIRC, I did a toggle button because it was the easiest for me to test with at the time. I will switch it to a preference. Ideas on the name?

@Mark-Simulacrum That's the bug I forgot to document. :oops: That bug will still be valid after I switch to a preference as well.

@core-ai-bot
Copy link
Member Author

Comment by Mark-Simulacrum
Monday Dec 22, 2014 at 03:16 GMT

@le717 My suggestion for the preference is useUppercaseColors if it only applies to colors, but I think it will apply to linear gradients and such in the future, which impedes this naming. Perhaps quick-edit.useUppercase is better, but not sure. Just a few thoughts.

@core-ai-bot
Copy link
Member Author

Comment by le717
Monday Dec 22, 2014 at 04:54 GMT

@redmunds The current code should be ready for an initial pass now.

@Mark-Simulacrum quick-edit.useUppercase would work, except there is CSS quick editor, color quick edit, easing curve quick edit, and soon linear-gradient quick edit, so it does not convey it applies only to the color-related quick editors.

@core-ai-bot
Copy link
Member Author

Comment by busykai
Monday Dec 22, 2014 at 13:16 GMT

@le717, there are JSHint errors in tests (xit is not defined).

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Monday Dec 22, 2014 at 14:32 GMT

I guess quick-edit.uppercaseColors or something along these lines is fine, as even for an potential gradient quick editor, only the color values would be uppercase. It'd be nice if it had another value for auto-detecting the case used in the editor.

I wonder if we should restrict uppercase language names to Hex colors only, as all the others look quite odd (haven't looked at the code though, so maybe this is already the case).

@core-ai-bot
Copy link
Member Author

Comment by le717
Monday Dec 22, 2014 at 16:34 GMT

@busykai Fully aware of the JSHint errors. Those are the new tests I initially added. I'll be reusing them once the reworking reaches a more final state. ;)

@MarcelGerber quick-edit.uppercaseColors sounds good to me.

It'd be nice if it had another value for auto-detecting the case used in the editor.

Yea, that would be a nice switch, but sounds like another preference-debate akin to
usePreferredOnly.

I wonder if we should restrict uppercase language names to Hex colors only, as all the others look quite odd (haven't looked at the code though, so maybe this is already the case).

I've gone back and forth on that. The feature request asked for making only Hex values upper/lowercase, but somewhere there are probably devs who write the other colors in uppercase too, so why should we restrict them from using their preferred case? Right now, the case preference applies universally.

@core-ai-bot
Copy link
Member Author

Comment by le717
Tuesday Dec 30, 2014 at 00:12 GMT

@redmunds Changes pushed. What else do you see?

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Tuesday Dec 30, 2014 at 01:16 GMT

Done with initial review.

@core-ai-bot
Copy link
Member Author

Comment by le717
Tuesday Dec 30, 2014 at 02:17 GMT

@redmunds Requested changes pushed, still have a comment on the preference listening.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Saturday Jan 03, 2015 at 18:31 GMT

@le717 This looks good, so you can work on the unit tests now.

@core-ai-bot
Copy link
Member Author

Comment by le717
Tuesday Jan 06, 2015 at 20:28 GMT

@redmunds Is there a test suite I can look at for reference? It seems that no matter what I try, I cannot get the test to set uppercaseColors to true.

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Tuesday Jan 06, 2015 at 20:33 GMT

The FileFilters test suite uses some preferences.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Tuesday Jan 06, 2015 at 21:38 GMT

@le717 Take a look in Editor-test.js at the "with soft tabs preference off" group of tests.

@core-ai-bot
Copy link
Member Author

Comment by le717
Wednesday Jan 07, 2015 at 01:35 GMT

Neither of those are working. :\ Is there a dependency or state that needs to be loaded/enabled first?

@core-ai-bot
Copy link
Member Author

Comment by le717
Monday Jan 12, 2015 at 22:58 GMT

@redmunds All changes except mixed-case tests made. However, the uppercase tests are still failing despite uppercaseColors confirmed to be true. I'm thinking it is an implementation error. I'll comment where I think it is coming in, but I am unsure of a fix ATM.

@core-ai-bot
Copy link
Member Author

Comment by le717
Tuesday Jan 13, 2015 at 16:49 GMT

@redmunds Test names, mixed case tests, and use getOriginalInput() changes pushed, all tests now pass. I didn't get your comment saying to use getOriginalInput() until later, so when I read your first one my thought was to use that method.

If everything looks good, I'll flatten this.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Tuesday Jan 13, 2015 at 18:38 GMT

@le717 Looks good! Squash it.

@core-ai-bot
Copy link
Member Author

Comment by le717
Wednesday Jan 14, 2015 at 01:57 GMT

@peterflynn@redmunds Listener hopefully patched, hopefully passes your inspection. Already squashed the commits.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Wednesday Jan 14, 2015 at 17:45 GMT

Done with review. This looks good. Just one last question about unit tests.

@core-ai-bot
Copy link
Member Author

Comment by le717
Friday Jan 16, 2015 at 18:00 GMT

Questions answered, switched unit tests to use getOriginalInput(). Ready on your command.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Friday Jan 16, 2015 at 19:23 GMT

Looks good. Merging.

@le717 Don't forget to add new preference to https://github.com/adobe/brackets/wiki/How-to-Use-Brackets#list-of-supported-preferences Note that prefs are now in alphabetic order.

@core-ai-bot
Copy link
Member Author

Comment by le717
Friday Jan 16, 2015 at 21:25 GMT

@redmunds Done. I wanted to add that this has the chance of breaking extension if they for whatever reason use the TinyColor version from here (which I don't think they do, they host their own) because of the API changes, so it might be good to list it as a library update in the change log.

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Saturday Jan 17, 2015 at 23:21 GMT

I just searched through all the extensions on the registry and luckily, there's not a single one using tinycolor. So we're pretty much good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot