search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

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

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by le717 Sunday Oct 19, 2014 at 00:34 GMT Originally opened as https://github.com/adobe/brackets/pull/9596

For #9519.

Features

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:

le717 included the following code: https://github.com/adobe/brackets/pull/9596/commits

core-ai-bot commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

@redmunds Changes pushed. What else do you see?

core-ai-bot commented 3 years ago

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

Done with initial review.

core-ai-bot commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

The FileFilters test suite uses some preferences.

core-ai-bot commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

@le717 Looks good! Squash it.

core-ai-bot commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.