feat(color-picker): add `clearable` prop and deprecate `allowEmpty` #8910

brandentheintern · 2024-03-08T18:29:32Z

Related Issue: #6880

Summary

Added clearable to contribute to continuity within the codebase - the logic allows for allowEmpty & clearable to be utilized to allow null values in the color picker.

jcfranco

Looking good, @brandentheintern! 😎

jcfranco · 2024-03-08T18:42:55Z

packages/calcite-components/src/components/color-picker/color-picker.stories.ts

@@ -69,6 +69,10 @@ export const simple = (): string =>
      name: "allow-empty",


This should be removed since we don't want to promote its use.

packages/calcite-components/src/components/color-picker/color-picker.tsx

jcfranco · 2024-03-08T18:45:08Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

@@ -99,6 +99,9 @@ export class ColorPicker
   *
   * When `false`, a color value is enforced, and clearing the input or blurring will restore the last valid `value`.
   */
+
+  @Prop({ reflect: true }) clearable = false;


Can you also move this so it's in alphabetical order within the properties section?

jcfranco · 2024-03-08T18:47:33Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

+    }
+  }
+
+  @Watch("clearable")


This and the above watcher have very similar code. Please consolidate into a single watcher to be used for both props (see Stencil @watch doc for example).

jcfranco · 2024-03-08T18:48:14Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

@@ -315,6 +336,8 @@ export class ColorPicker

  private shiftKeyChannelAdjustment = 0;

+  private allowNullValues = !!(this.clearable || this.allowEmpty);


I know we talked about this, but let's make sure this variable is assigned in a lifecycle method vs when the component is instantiated. This works fine in the lazy-loaded output target, but does not get properly assigned in the components (tree-shakeable) output target.

jcfranco · 2024-03-08T18:49:09Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

@@ -677,7 +699,7 @@ export class ColorPicker
    if (!valueIsCompatible) {
      this.showIncompatibleColorWarning(value, format);
    }
-
+    console.log(allowNullValues);


Please remove log statements.

jcfranco · 2024-03-08T18:49:48Z

packages/calcite-components/src/demos/color-picker.html

@@ -49,7 +49,7 @@ <h1 style="margin: 0 auto; text-align: center">Color picker</h1>
      <div class="parent">
        <div class="child right-aligned-text">Default</div>
        <div class="child">
-          <calcite-color-picker scale="s"></calcite-color-picker>
+          <calcite-color-picker clearable scale="s"></calcite-color-picker>


This is unrelated to the existing demo block. Please revert.

jcfranco · 2024-03-08T18:52:51Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

@@ -315,6 +336,8 @@ export class ColorPicker

  private shiftKeyChannelAdjustment = 0;

+  private allowNullValues = !!(this.clearable || this.allowEmpty);


Can we rename this to isClearable, canClear or similar to match the new prop name? This would avoid having 3 different ways of referring to the same behavior (e.g., clear + empty vs null + clear + empty).

…or-picker-clearable

jcfranco

Awesome! One more pass and I think this will be good to merge! 🚀👨‍🚀🪐

jcfranco · 2024-03-14T23:11:10Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

  @Prop({ reflect: true }) allowEmpty = false;

+  @Watch("allowEmpty")
+  @Watch("clearable")
+  handleEmptyValues(): void {


Can you revisit this name? IMO, it doesn't convey what it's doing.

jcfranco · 2024-03-14T23:12:10Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

@@ -98,17 +98,31 @@ export class ColorPicker
   * When `true`, an empty color (`null`) will be allowed as a `value`.
   *
   * When `false`, a color value is enforced, and clearing the input or blurring will restore the last valid `value`.
+   *
+   * @deprecated use `clearable` instead


Can you capitalize use? I'll submit a separate PR to ensure we capitalize deprecation messages.

jcfranco · 2024-03-14T23:12:27Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

+   *
+   * When `false`, a color value is enforced, and clearing the input or blurring will restore the last valid `value`.
+   */
+


For consistency, can you remove this newline?

jcfranco · 2024-03-14T23:13:18Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

+   * When `false`, a color value is enforced, and clearing the input or blurring will restore the last valid `value`.
+   */
+
+  @Prop({ reflect: true }) clearable = false;


Can you move this prop and associated doc so that it is sorted alphabetically within the properties section?

packages/calcite-components/src/components/color-picker/color-picker.tsx

jcfranco · 2024-03-14T23:16:16Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

@@ -293,6 +310,8 @@ export class ColorPicker
    return this.color || this.previousColor || DEFAULT_COLOR;
  }

+  private isClearable: boolean;


Can you move this so that it is in alphabetical order within its fellow private vars?

jcfranco · 2024-03-18T15:35:50Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

@@ -305,6 +320,8 @@ export class ColorPicker

  private internalColorUpdateContext: "internal" | "initial" | "user-interaction" | null = null;

+  private isClearable: boolean = !!(this.clearable || this.allowEmpty);


Per our previous conversation, isClearable should be assigned in a lifecycle hook.

jcfranco · 2024-03-18T15:36:19Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

+  @Watch("allowEmpty")
+  @Watch("clearable")
+  handleAllowNullValues(): void {
+    this.isClearable = !!(this.clearable || this.allowEmpty);


The !! is unnecessary as you are using two boolean properties with default values.

jcfranco · 2024-03-18T15:42:15Z

packages/calcite-components/src/components/color-picker/color-picker.tsx

   */
  @Prop({ reflect: true }) allowEmpty = false;

+  @Watch("allowEmpty")
+  @Watch("clearable")
+  handleAllowNullValues(): void {


I'd like to avoid using "null values" and instead use a name for the watcher that matches the watched properties. Some suggestions:

handleAllowClearingPropChange

handleAllowEmptyOrClearableChange

jcfranco

🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️
🖌️🎨🎨🎨🎨🖌️🎨🖌️🖌️🖌️🖌️🖌️🎨🖌️🎨🎨🎨🎨🖌️🖌️🎨🎨🎨🖌️🖌️🎨🎨🖌️🖌️🎨🖌️🖌️🖌️🎨🖌️🎨🎨🎨🎨🖌️🎨🖌️
🖌️🎨🖌️🖌️🎨🖌️🎨🖌️🖌️🖌️🖌️🖌️🎨🖌️🎨🖌️🖌️🖌️🖌️🎨🖌️🖌️🖌️🖌️🎨🖌️🖌️🎨🖌️🎨🎨🖌️🎨🎨🖌️🎨🖌️🖌️🖌️🖌️🎨🖌️
🖌️🎨🎨🎨🎨🖌️🎨🖌️🖌️🎨🖌️🖌️🎨🖌️🎨🎨🎨🖌️🖌️🖌️🎨🎨🖌️🖌️🎨🖌️🖌️🎨🖌️🎨🖌️🎨🖌️🎨🖌️🎨🎨🎨🖌️🖌️🎨🖌️
🖌️🎨🖌️🖌️🎨🖌️🎨🖌️🎨🖌️🎨🖌️🎨🖌️🎨🖌️🖌️🖌️🖌️🖌️🖌️🖌️🎨🖌️🎨🖌️🖌️🎨🖌️🎨🖌️🖌️🖌️🎨🖌️🎨🖌️🖌️🖌️🖌️🖌️🖌️
🖌️🎨🖌️🖌️🎨🖌️🖌️🎨🖌️🖌️🖌️🎨🖌️🖌️🎨🎨🎨🎨🖌️🎨🎨🎨🖌️🖌️🖌️🎨🎨🖌️🖌️🎨🖌️🖌️🖌️🎨🖌️🎨🎨🎨🎨🖌️🎨🖌️
🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️🖌️

🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-design-tokens: 2.1.2</summary> ## [2.1.2](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2024-03-26) ### Bug Fixes * Fix invalid font stacks ([#8964](#8964)) ([d55186a](d55186a)) </details> <details><summary>@esri/calcite-components: 2.7.0</summary> ## [2.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2024-03-26) ### Features * **button:** Add download property ([#8882](#8882)) ([fc55dde](fc55dde)) * **color-picker:** Add `clearable` prop and deprecate `allowEmpty` ([#8910](#8910)) ([f036ac2](f036ac2)) * **table-row:** Add alignment property ([#8961](#8961)) ([1f81fd7](1f81fd7)) * **tabs:** Make component responsive ([#8616](#8616)) ([83a2ef4](83a2ef4)) * **tile:** Add content-top and content-bottom slots, deprecate content-start and content-end slots ([#8984](#8984)) ([eb000d8](eb000d8)) ### Bug Fixes * **action-menu, combobox, dropdown, popover, tooltip:** Use click instead of pointerdown for click contexts ([#8943](#8943)) ([cd7eed9](cd7eed9)) * **card:** Do not apply text color to slotted footer items ([#8839](#8839)) ([30a2068](30a2068)) * **combobox:** Prevent spacebar from opening the menu when focused on chip's close button ([#8990](#8990)) ([1a20d0e](1a20d0e)) * **dropdown:** Correct positioning behavior when inside a scrollable container ([#8973](#8973)) ([2524391](2524391)) * **input-time-picker:** Update toggle icon color ([#8955](#8955)) ([ce3ac5c](ce3ac5c)) * **input, input-number, input-text:** Ensure values are initialized properly for dist and components output targets ([#8997](#8997)) ([9152211](9152211)) * **list, list-item:** Calcite-select becomes unresponsive in a list-item if drag-disabled is true ([#8957](#8957)) ([e408c62](e408c62)) * **list:** Fix filter box when scrolling in Safari ([#8938](#8938)) ([ea8ea1e](ea8ea1e)) * **popover:** Prevent disabled reference elements from toggling popover ([#8983](#8983)) ([9f4b14b](9f4b14b)) * **stepper:** Fix layout when switching modes dynamically to `horizontal-single` ([#8946](#8946)) ([01f58bf](01f58bf)) * **table:** Prevent duplicate scrollbars in certain browsers ([#8962](#8962)) ([8434eeb](8434eeb)) * **tab:** Style focus outline of tab content ([#8804](#8804)) ([8f0133f](8f0133f)) ### Performance Improvements * Update `transition-default` Tailwind util to only target common, animatable properties ([#8797](#8797)) ([f4d016b](f4d016b)) ### Reverts * Refactor(popover): update token pattern ([#8889](#8889)) ([c43c280](c43c280)) ### Dependencies * The following workspace dependencies were updated * devDependencies * @esri/calcite-design-tokens bumped from ^2.1.2-next.2 to ^2.1.2 </details> <details><summary>@esri/calcite-components-angular: 2.7.0</summary> ## [2.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2024-03-26) ### Miscellaneous Chores * **@esri/calcite-components-angular:** Synchronize components versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.7.0-next.17 to ^2.7.0 </details> <details><summary>@esri/calcite-components-react: 2.7.0</summary> ## [2.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2024-03-26) ### Miscellaneous Chores * **@esri/calcite-components-react:** Synchronize components versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.7.0-next.17 to ^2.7.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

test(color-picker): added clearable logic alongside allowEmpty

db87fef

brandentheintern requested a review from a team as a code owner March 8, 2024 18:29

github-actions bot assigned brandentheintern Mar 8, 2024

github-actions bot added the testing Issues related to automated or manual testing. label Mar 8, 2024

brandentheintern changed the title ~~test(color-picker): added clearable logic alongside allowEmpty~~ feat(color-picker): added clearable logic alongside allowEmpty Mar 8, 2024

brandentheintern changed the title ~~feat(color-picker): added clearable logic alongside allowEmpty~~ feat(color-picker): add new prop clearable and deprecated prop allowEmpty Mar 8, 2024

brandentheintern changed the title ~~feat(color-picker): add new prop clearable and deprecated prop allowEmpty~~ feat(color-picker): add prop clearable and deprecate allowEmpty Mar 8, 2024

brandentheintern changed the title ~~feat(color-picker): add prop clearable and deprecate allowEmpty~~ feat(color-picker): add clearable prop and deprecate allowEmpty Mar 8, 2024

brandentheintern changed the title ~~feat(color-picker): add clearable prop and deprecate allowEmpty~~ feat(color-picker): add clearable prop and deprecate allowEmpty Mar 8, 2024

jcfranco requested changes Mar 8, 2024

View reviewed changes

brandentheintern added 3 commits March 12, 2024 15:17

Merge branch 'main' of github.com:Esri/calcite-design-system into col…

8c46f48

…or-picker-clearable

feat(color-picker): clean up isClear code

b78503d

feat(color-picker): update isClearable to follow conventions

66867f8

github-actions bot added the enhancement Issues tied to a new feature or request. label Mar 14, 2024

jcfranco requested changes Mar 14, 2024

View reviewed changes

brandentheintern added 2 commits March 15, 2024 11:53

feat(color-picker): add clearable prop and deprecate allowEmpty

2e43f30

feat(color-picker): add clearable prop and deprecate allowEmpty

5e844a2

jcfranco reviewed Mar 18, 2024

View reviewed changes

jcfranco mentioned this pull request Mar 18, 2024

refactor(color-picker): add clearable property and deprecate allowEmpty #8904

Closed

brandentheintern added 2 commits March 18, 2024 16:17

feat(color-picker): add clearable prop and deprecate allowEmpty

e6acc84

feat(color-picker): add clearable prop and deprecate allowEmpty

1d1619a

brandentheintern added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 19, 2024

jcfranco approved these changes Mar 19, 2024

View reviewed changes

brandentheintern merged commit f036ac2 into main Mar 19, 2024
15 checks passed

brandentheintern deleted the color-picker-clearable branch March 19, 2024 18:04

github-actions bot added this to the 2024-03-26 - Mar Release milestone Mar 19, 2024

calcite-admin mentioned this pull request Mar 19, 2024

chore: release main #8842

Merged

calcite-admin mentioned this pull request May 14, 2024

chore: release main #9237

Merged

calcite-admin mentioned this pull request May 28, 2024

chore: release hotfix #9438

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(color-picker): add `clearable` prop and deprecate `allowEmpty` #8910

feat(color-picker): add `clearable` prop and deprecate `allowEmpty` #8910

brandentheintern commented Mar 8, 2024

jcfranco left a comment

jcfranco Mar 8, 2024

jcfranco Mar 8, 2024

jcfranco Mar 8, 2024

jcfranco Mar 8, 2024

jcfranco Mar 8, 2024

jcfranco Mar 8, 2024

jcfranco Mar 8, 2024

jcfranco left a comment

jcfranco Mar 14, 2024

jcfranco Mar 14, 2024

jcfranco Mar 14, 2024 •

edited

Loading

jcfranco Mar 14, 2024

jcfranco Mar 14, 2024

jcfranco Mar 18, 2024

jcfranco Mar 18, 2024

jcfranco Mar 18, 2024

jcfranco left a comment

		@@ -69,6 +69,10 @@ export const simple = (): string =>
		name: "allow-empty",

		@@ -315,6 +336,8 @@ export class ColorPicker

		private shiftKeyChannelAdjustment = 0;

		private allowNullValues = !!(this.clearable \|\| this.allowEmpty);

		@@ -305,6 +320,8 @@ export class ColorPicker

		private internalColorUpdateContext: "internal" \| "initial" \| "user-interaction" \| null = null;

		private isClearable: boolean = !!(this.clearable \|\| this.allowEmpty);

feat(color-picker): add clearable prop and deprecate allowEmpty #8910

feat(color-picker): add clearable prop and deprecate allowEmpty #8910

Conversation

brandentheintern commented Mar 8, 2024

Summary

jcfranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfranco Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfranco left a comment

Choose a reason for hiding this comment

feat(color-picker): add `clearable` prop and deprecate `allowEmpty` #8910

feat(color-picker): add `clearable` prop and deprecate `allowEmpty` #8910

jcfranco Mar 14, 2024 •

edited

Loading