feat(list, list-item): add non-interactive option to remove hover/pointer changes. #10473

aPreciado88 · 2024-10-01T22:56:58Z

Related Issue: #8634

Summary

Add interaction-mode="interactive" (default) | "static" prop.
Setting interaction-mode="static" and selection-appearance="icon" removes hover/pointer changes from main content area.
Setting interaction-mode="static" and selection-appearance="border" falls back to interactive hover/pointer changes and logs console warning message.
Setting interaction-mode="static" and selection-mode="none" removes hover/pointer changes.

…nter changes

driskull

Can we move the warning log to the list?

driskull · 2024-10-07T15:56:35Z

packages/calcite-components/src/components/list-item/list-item.tsx

+  @Watch("interactionMode")
+  @Watch("selectionAppearance")
+  @Watch("selectionMode")
+  handleInteractionModeSelectionAppearanceSelectionModeChange(): void {


I think this would need to be called on componentWillLoad as well, otherwise it would only warn after one of these was changed.

Should this warning occur on the list instead of the list item? The property on the list-item is internal so its weird to log here.

I moved this logic into list.tsx 🚀

driskull

Code looks good! can we just not reflect the internal prop?

driskull · 2024-10-11T18:57:29Z

packages/calcite-components/src/components/list-item/list-item.tsx

+   * Specifies the interaction mode of the component - `"interactive"` (allows interaction styling and pointer changes on hover), `"static"` (does not allow interaction styling and pointer changes on hover), Do not combine `interaction-mode=“static”` and `selection-appearance=“border”`.
+   * @internal
+   */
+  @Prop({ reflect: true }) interactionMode: InteractionMode = null;


Does this need to be reflected? Since its internal and not being used for a selector can we not reflect it?

Thanks for catching this! I updated it to not be reflected.

driskull · 2024-10-11T18:59:02Z

packages/calcite-components/src/components/list/list.tsx

+      this.selectionMode !== "none" &&
+      this.selectionAppearance === "border"
+    ) {
+      console.warn(`selection-appearance=“border” requires interaction-mode=“interactive”`);


Do we need the warning or will good documentation suffice? Just questioning if this is indeed necessary.

This only warns on first use so if someone set these invalid combination after the fact there would be no warning. IMO we can just rely on doc for this but would love others thoughts.

Aaron, Franco and me went over the issue, and landed on setting a detailed JSDoc for documentation while also logging the warning message. But maybe this can be reconsidered?
cc @jcfranco @ashetland

I defer to @jcfranco and @driskull.

Great question. Since this is something teams have been asking for, let’s keep the warning in place to ensure developers use the proper configuration. After #10310, we can explore showing this and related warnings (e.g., color-picker’s incompatible value + format mode) only in dev builds, as suggested in #10038.

aPreciado88 · 2024-10-11T19:41:46Z

@ashetland @SkyeSeitz Can you please take a look at the new snapshots?

jcfranco

Good to merge after addressing feedback.

👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍
👍✅👍👍👍✅👍👍✅✅👍👍✅✅✅👍👍✅✅✅👍✅✅✅✅👍✅👍
👍✅✅👍👍✅👍✅👍👍✅👍👍✅👍👍✅👍👍👍👍✅👍👍👍👍✅👍
👍✅👍✅👍✅👍✅👍👍✅👍👍✅👍👍✅👍👍👍👍✅✅✅👍👍✅👍
👍✅👍👍✅✅👍✅👍👍✅👍👍✅👍👍✅👍👍👍👍✅👍👍👍👍👍👍
👍✅👍👍👍✅👍👍✅✅👍👍✅✅✅👍👍✅✅✅👍✅✅✅✅👍✅👍
👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍

jcfranco · 2024-10-18T16:21:14Z

packages/calcite-components/src/components/list/list.tsx

+   *
+   * `"static"` does not allow interaction styling and pointer changes on hover
+   *
+   * Do not combine `interaction-mode=“static”` and `selection-appearance=“border”`


+@geospatialem @DitwanP for editorial review. 📖👀

@aPreciado88 Can you speak more to the "why" behind not using the props together? Maybe something we can also add to enhance the component's documentation page. I didn't see context in the PR's summary or in the console warning, so wanted to be sure I understand the "why" a bit more before providing a review. Did observe different double quotes, so recommend changing those in the interim.

Suggested change

* Do not combine `interaction-mode=“static”` and `selection-appearance=“border”`

* Do not combine `interaction-mode="static"` and `selection-appearance="border"`

@geospatialem The reason for the console warning and not using the props together is described in this issue. Under Calcite Design Solution > selection-appearance=“border”.

Summarizing our discussions via chat:

We'll enhance the above to mention it is recommended to only add the interaction-mode="static" and "selection-appearance="none" together (see suggestion below)

We'll provide more context via some images to the list's doc page under a new "Recommendations" section cc: @ashetland @jcfranco

Suggested change

* Do not combine `interaction-mode=“static”` and `selection-appearance=“border”`

* The `"static"` value should only be used when `selectionAppearance` is `"none"`.

The property should be selection-mode, not selection-appearance.

Updated the PR with the requested changes. Thanks for catching that @ashetland!

jcfranco · 2024-10-18T16:26:00Z

packages/calcite-components/src/components/list/list.stories.ts

@@ -1137,3 +1150,23 @@ export const dragEnabledNestedListsIndirectChildren = (): string =>
    <div><calcite-list-item label="Depth 1" description="Item 6"></calcite-list-item></div>
    <div><calcite-list-item drag-disabled label="Depth 1" description="Item 7"></calcite-list-item></div>
  </calcite-list>`;
+
+export const nonInteractive = (): string => html`


Can you merge these stories into one with a descriptor for each use case (e.g., slider fill placements)?

jcfranco · 2024-10-18T16:26:21Z

packages/calcite-components/src/components/list/list.stories.ts

@@ -1137,3 +1150,23 @@ export const dragEnabledNestedListsIndirectChildren = (): string =>
    <div><calcite-list-item label="Depth 1" description="Item 6"></calcite-list-item></div>
    <div><calcite-list-item drag-disabled label="Depth 1" description="Item 7"></calcite-list-item></div>
  </calcite-list>`;
+
+export const nonInteractive = (): string => html`
+  <div style="width: 400px">


Nitpick: is setting width necessary for this test?

This is not necessary, I can take the width out.

jcfranco · 2024-10-18T16:36:12Z

packages/calcite-components/src/components/list-item/list-item.tsx

@@ -682,7 +696,7 @@ export class ListItem
              aria-setsize={setSize}
              class={{
                [CSS.container]: true,
-                [CSS.containerHover]: true,
+                [CSS.containerHover]: !containerNotInteractive || containerInteractive,


This naming a bit misleading. Since containerInteractive and containerNotInteractive are opposites, this ends up reading as containerInteractive || containerInteractive.

jcfranco · 2024-10-18T16:41:45Z

packages/calcite-components/src/components/list-item/list-item.tsx

@@ -668,6 +675,13 @@ export class ListItem
    const showBorder = selectionMode !== "none" && selectionAppearance === "border";
    const borderSelected = showBorder && selected;
    const borderUnselected = showBorder && !selected;
+    const containerNotInteractive =


I think this can be simplified to:

const containerInteractive = interactionMode === "interactive" || ( interactionMode === "static" && selectionMode !== "none" && selectionAppearance === "border" ); // ... [CSS.containerHover]: containerInteractive,

Thanks for catching that, this update works!

ashetland · 2024-10-18T20:14:52Z

Lookin' good!

…nter changes. (#10473) **Related Issue:** [#8634](#8634) ## Summary - Add `interaction-mode="interactive" (default) | "static"` prop. - Setting `interaction-mode="static"` and `selection-appearance="icon"` removes hover/pointer changes from main content area. - Setting `interaction-mode="static"` and `selection-appearance="border"` falls back to interactive hover/pointer changes and logs console warning message. - Setting `interaction-mode="static"` and `selection-mode="none"` removes hover/pointer changes.

aPreciado88 added 2 commits October 1, 2024 15:39

feat(list, list-item): add non-interactive option to remove hover/poi…

138e6f2

…nter changes

feat(list, list-item): fix merge conflicts

fb6c609

github-actions bot assigned aPreciado88 Oct 1, 2024

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

aPreciado88 added this to the 2024-10-29 - Oct Milestone milestone Oct 1, 2024

feat(list, list-item): update e2e test

8566f97

aPreciado88 added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 3, 2024

aPreciado88 requested review from jcfranco and driskull October 4, 2024 22:02

driskull requested changes Oct 7, 2024

View reviewed changes

feat(list, list-item): move warning logger into list.tsx

ece16ba

aPreciado88 requested a review from driskull October 11, 2024 18:36

driskull approved these changes Oct 11, 2024

View reviewed changes

feat(list, list-item): remove reflect from internal interactionMode prop

6175655

aPreciado88 added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 11, 2024

feat(list, list-item): disable interactivity when selection-mode=none

26f63e4

jcfranco approved these changes Oct 18, 2024

View reviewed changes

feat(list, list-item): update interactive list item logic and e2e tests

69d4258

aPreciado88 added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 18, 2024

ashetland self-requested a review October 18, 2024 20:15

ashetland approved these changes Oct 18, 2024

View reviewed changes

aPreciado88 added 2 commits October 21, 2024 12:30

feat(list, list-item): update jsdoc description

b0648c6

feat(list, list-item): update console warning quotes

c1203db

aPreciado88 added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 21, 2024

aPreciado88 added the skip visual snapshots Pull requests that do not need visual regression testing. label Oct 21, 2024

aPreciado88 merged commit c149b55 into dev Oct 21, 2024
17 checks passed

aPreciado88 deleted the aPreciado88/8634-add-non-interactive-option-to-remove-pointer-and-hover-changes branch October 21, 2024 20:51

	* Do not combine `interaction-mode=“static”` and `selection-appearance=“border”`
	* Do not combine `interaction-mode="static"` and `selection-appearance="border"`

	* Do not combine `interaction-mode=“static”` and `selection-appearance=“border”`
	* The `"static"` value should only be used when `selectionAppearance` is `"none"`.

feat(list, list-item): add non-interactive option to remove hover/pointer changes. #10473

feat(list, list-item): add non-interactive option to remove hover/pointer changes. #10473

Uh oh!

Conversation

aPreciado88 commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aPreciado88 commented Oct 11, 2024

Uh oh!

jcfranco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aPreciado88 Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashetland commented Oct 18, 2024

Uh oh!

Uh oh!

Uh oh!

aPreciado88 commented Oct 1, 2024 •

edited

Loading

aPreciado88 Oct 21, 2024 •

edited

Loading