feat(s2): coachmarks #7590

jluyau · 2025-01-09T18:36:31Z

Closes

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

rspbot · 2025-01-09T20:01:38Z

Build successful! 🎉

rspbot · 2025-01-09T20:40:05Z

Build successful! 🎉

rspbot · 2025-01-15T02:09:07Z

Build successful! 🎉

snowystinger

I think the Coachmark should be a Focus trap. Probably just need to include a Dialog inside the Popover.

Coachmarks and indicator look pretty :) have a couple initial big points

snowystinger · 2025-01-15T02:03:45Z

packages/@react-spectrum/s2/stories/CoachMark.stories.tsx

+export default meta;
+
+export const CoachMarkExample = (args) => (
+  <CoachMarkTrigger isOpen>


I don't think this is the API we want. Otherwise Coachmarks are always in the JSX and it becomes hard to disentangle them or coordinate over an entire application for a tour. For instance, a Coachmark may not be rendered until a lazy component has loaded or maybe a view is swapped out part way through the tour.

Instead, we should use the same strategy from v2 and attach to selectors with a retry pattern for finding the current step.

snowystinger · 2025-01-15T02:05:11Z

packages/@react-spectrum/s2/stories/CoachMark.stories.tsx

+import TextBold from '../s2wf-icons/S2_Icon_TextBold_20_N.svg';
+import TextIcon from '../s2wf-icons/S2_Icon_Text_20_N.svg';
+import {Toolbar} from 'react-aria-components';
+


stories should all have a button to start the tour, I don't think the tour should auto start when you enter the docs otherwise there could be multiple tours at once

jluyau · 2025-01-15T03:10:26Z

I think the Coachmark should be a Focus trap. Probably just need to include a Dialog inside the Popover.

yeah i initially went with a focus trap, but looking at the Spectrum design docs, there's a type of coach mark that will only advance if the element it's modifying is interacted with (e.g. a button is pressed), and i wasn't able to tap the element with the coach mark focusables.

snowystinger · 2025-01-15T05:46:18Z

I think the Coachmark should be a Focus trap. Probably just need to include a Dialog inside the Popover.

yeah i initially went with a focus trap, but looking at the Spectrum design docs, there's a type of coach mark that will only advance if the element it's modifying is interacted with (e.g. a button is pressed), and i wasn't able to tap the element with the coach mark focusables.

oh... no....

That would be difficult to detect from a global coordination standpoint, so I see why you opted to try and place the coachmark using the target as the trigger element.

Would all the "clickable" things even be buttons? What if it's telling you how to paint on a canvas?

If we tried to do that, we'd need something better than a capturing click listener since we stop propagation and we also will be moving to firing onPress in the click event IF it even exists. Different forms of interaction may not even fire click, for instance keyboard.

I think the better thing to do would be to have the UI update state in response to the tour moving along, regardless of clicking on a button or canvas.

Maybe bring these different use cases to an Otters time, I suspect there will be opinions.

packages/@react-spectrum/s2/src/CoachMark.tsx

devongovett · 2025-01-28T20:09:26Z

packages/@react-spectrum/s2/stories/CoachMark.stories.tsx

+          placement="left top"
+          {...otherTourProps}
+          description="This is the Apply button. Press it to continue the tour."
+          hasPressAction />


This is really strange IMO. I guess it's overriding the press events on the button? Does the button also do its normal action?

snowystinger

Notes for future work:
Add docs to use https://react-spectrum.adobe.com/react-aria/Popover.html#controlled-open-state to organise a tour or A/B testing more easily. Otherwise default usage of CoachmarkTrigger is fine.

# Conflicts: # packages/@react-spectrum/s2/package.json # yarn.lock

rspbot · 2025-03-11T00:27:39Z

Build successful! 🎉

snowystinger · 2025-04-10T01:54:18Z

I've added UNSTABLE prefix and removed it from the auto docs so we can get this merged

rspbot · 2025-04-10T02:04:41Z

Build successful! 🎉

rspbot · 2025-04-10T04:16:27Z

Build successful! 🎉

rspbot · 2025-04-11T03:30:25Z

Build successful! 🎉

# Conflicts: # packages/@react-spectrum/s2/package.json # yarn.lock

rspbot · 2025-04-15T06:11:22Z

Build successful! 🎉

devongovett · 2025-04-18T00:20:11Z

packages/@react-spectrum/s2/package.json

    "@react-aria/utils": "^3.28.2",
    "@react-spectrum/utils": "^3.12.4",
    "@react-stately/layout": "^4.2.2",
+    "@react-stately/menu": "^3.9.3",


Looks like these dependencies aren't used anywhere?

https://github.com/adobe/react-spectrum/pull/7590/files#diff-8cd68236c9fcef8c0976a16402d56ed41af3bbdac40c17b02ff088920c1d51deR52

wanted to revisit this in followup because I'm not sure we should allow menus in a coachmark

sorry, wrong one, @react-aria/overlays is unused.

devongovett · 2025-04-18T00:21:18Z

packages/@react-spectrum/s2/src/CoachMark.tsx

+  size?: 'S' | 'M' | 'L' | 'XL'
+}
+
+const fadeKeyframes = keyframes(`


Assuming all of this will change in a future PR, but we shouldn't need these anymore now that popover supports CSS transitions. Popover in S2 was updated in the meantime.

not using S2 Popover in followup, see https://github.com/adobe/react-spectrum/pull/8095/files#diff-bc1a3b22ba9ce958f364c9100ab798d2a606a2e24c34e5a9d4a3d45cdcbb2107

devongovett · 2025-04-18T00:21:33Z

packages/@react-spectrum/s2/src/CoachMark.tsx

+  }
+`);
+
+let popover = style({


Would be nice to avoid duplicating this...

yeah, i got it a lot smaller in my followup, can see about getting rid of it there

devongovett · 2025-04-18T00:23:08Z

packages/@react-spectrum/s2/src/CoachMark.tsx

+        zIndex: undefined
+      }}
+      className={(renderProps) =>  mergeStyles(popover({...renderProps, colorScheme}))}>
+      <Card>


I guess you're planning to change this later?

yeah, in my follow up i just have CoachMark as an empty container
https://github.com/adobe/react-spectrum/pull/8095/files#diff-bf811e14d299f5b052ff23dae48c30e7808ab7d09672bc60eeac784952161d58R153

devongovett · 2025-04-18T00:24:28Z

packages/@react-spectrum/s2/src/CoachMark.tsx

+    value: {
+      default: '[-2px]',
+      ':has([data-trigger=checkbox])': '[-6px]',
+      ':has([data-trigger=slider])': '[-8px]',


I guess we probably want to avoid special values for specific components.

Yeah I haven't thought of a good way to do this yet. I was going to do like we discussed and search for the first border radius, but that had problems. See https://github.com/adobe/react-spectrum/pull/8095/files#diff-8cd68236c9fcef8c0976a16402d56ed41af3bbdac40c17b02ff088920c1d51deR264

devongovett · 2025-04-18T00:24:58Z

packages/@react-spectrum/s2/src/CoachMark.tsx

+  }
+});
+
+const pulse = raw(`&:before { content: ""; display: inline-block; position: absolute; top: var(--borderOffset); bottom:  var(--borderOffset); left: var(--borderOffset); right: var(--borderOffset); border-radius: var(--ringRadius); outline-style: solid; outline-color: var(--activeElement); outline-width: 4px; animation-duration: 2s; animation-iteration-count: infinite; animation-timing-function: ease-in-out; animation-fill-mode: forwards; animation-name: ${pulseAnimation}}`);


Can it be an actual element? Why is it a before?

devongovett · 2025-04-18T00:25:34Z

packages/@react-spectrum/s2/src/index.ts

@@ -27,6 +27,7 @@ export {CardView, CardViewContext} from './CardView';
 export {Checkbox, CheckboxContext} from './Checkbox';
 export {CheckboxGroup, CheckboxGroupContext} from './CheckboxGroup';
 export {CloseButton} from './CloseButton';
+export {CoachMark as UNSTABLE_CoachMark, CoachMarkTrigger as UNSTABLE_CoachMarkTrigger} from './CoachMark';


Let's not export it for now. This seems like pre-unstable :D

rspbot · 2025-04-18T01:21:00Z

Build successful! 🎉

rspbot · 2025-04-18T01:31:43Z

Build successful! 🎉

jluyau added 7 commits December 18, 2024 16:51

GW coachmark

e4522ee

fix TS errors

d42a8ee

Merge branch 'main' of github.com:adobe/react-spectrum into coachmarks

fb42457

remove unrelated files, update yarn.lock

3b7f7bf

remove unneeded import

a104309

lint fix

1162a44

more lint fixes

8cbcadf

jluyau added 2 commits January 9, 2025 12:09

remove old stories

a33a533

remove unused import

6c01220

Merge branch 'main' into coachmarks

0512bee

snowystinger reviewed Jan 15, 2025

View reviewed changes

devongovett reviewed Jan 28, 2025

View reviewed changes

snowystinger reviewed Jan 30, 2025

View reviewed changes

snowystinger added 2 commits March 11, 2025 09:53

Merge branch 'main' into coachmarks

e0eca9e

# Conflicts: # packages/@react-spectrum/s2/package.json # yarn.lock

fix yarn lock

3601365

snowystinger added 4 commits March 20, 2025 11:18

Merge branch 'main' into coachmarks

5c252ef

API changes to bring in line with other s2 components

7fb0ea7

Cleanup to get merged

be9343b

fix lint and tests

7f0f308

snowystinger changed the title ~~feat(s2): coachmarks [WIP]~~ feat(s2): coachmarks Apr 10, 2025

Merge branch 'main' into coachmarks

109d95f

Merge branch 'main' into coachmarks

68ac81a

Merge branch 'main' into coachmarks

b7f9e10

# Conflicts: # packages/@react-spectrum/s2/package.json # yarn.lock

snowystinger added the in sprint label Apr 16, 2025

devongovett reviewed Apr 18, 2025

View reviewed changes

remove from exports entirely

f6911aa

Merge branch 'main' into coachmarks

0041a72

devongovett approved these changes Apr 18, 2025

View reviewed changes

snowystinger approved these changes Apr 22, 2025

View reviewed changes

snowystinger added this pull request to the merge queue Apr 22, 2025

Merged via the queue into main with commit f76c445 Apr 22, 2025
30 checks passed

snowystinger deleted the coachmarks branch April 22, 2025 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s2): coachmarks #7590

feat(s2): coachmarks #7590

jluyau commented Jan 9, 2025

rspbot commented Jan 9, 2025

rspbot commented Jan 9, 2025

rspbot commented Jan 15, 2025

snowystinger left a comment

snowystinger Jan 15, 2025

snowystinger Jan 15, 2025

jluyau commented Jan 15, 2025

snowystinger commented Jan 15, 2025

devongovett Jan 28, 2025

snowystinger left a comment

rspbot commented Mar 11, 2025

snowystinger commented Apr 10, 2025

rspbot commented Apr 10, 2025

rspbot commented Apr 10, 2025

rspbot commented Apr 11, 2025

rspbot commented Apr 15, 2025

devongovett Apr 18, 2025

snowystinger Apr 18, 2025

devongovett Apr 18, 2025

devongovett Apr 18, 2025

snowystinger Apr 18, 2025

devongovett Apr 18, 2025

snowystinger Apr 18, 2025

devongovett Apr 18, 2025

snowystinger Apr 18, 2025

devongovett Apr 18, 2025

snowystinger Apr 18, 2025

devongovett Apr 18, 2025

devongovett Apr 18, 2025

rspbot commented Apr 18, 2025

rspbot commented Apr 18, 2025

feat(s2): coachmarks #7590

feat(s2): coachmarks #7590

Conversation

jluyau commented Jan 9, 2025

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Jan 9, 2025

rspbot commented Jan 9, 2025

rspbot commented Jan 15, 2025

snowystinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jluyau commented Jan 15, 2025

snowystinger commented Jan 15, 2025

Choose a reason for hiding this comment

snowystinger left a comment

Choose a reason for hiding this comment

rspbot commented Mar 11, 2025

snowystinger commented Apr 10, 2025

rspbot commented Apr 10, 2025

rspbot commented Apr 10, 2025

rspbot commented Apr 11, 2025

rspbot commented Apr 15, 2025

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

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

rspbot commented Apr 18, 2025

rspbot commented Apr 18, 2025