Adding ListView chromatic stories #3106

LFDanLu · 2022-05-05T22:47:25Z

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:

Look at the chromatic tests for ListView and confirm behavior

🧢 Your Project:

RSP

adobe-bot · 2022-05-05T22:53:01Z

Build successful! 🎉

LFDanLu · 2022-05-05T22:54:14Z

packages/@react-types/actiongroup/src/index.d.ts

+  /**
+   * A slot to place the ActionGroup in.
+   * @default 'actionGroup'
+   */
+  slot?: string


Not entirely sure if this slot prop should be exposed, it all depends on our slot naming consistency and whether or not a ActionGroup could be placed in a slot other than actionGroup. I fine with removing this for now and adding it back in when we have a need

Same applies for ActionMenu below

makes sense, it should be here

Where is this slot being used by ListView?

This slot is used here:

react-spectrum/packages/@react-spectrum/list/src/ListViewItem.tsx

Lines 219 to 223 in 8445498

actionGroup: {

UNSAFE_className: listStyles['react-spectrum-ListViewItem-actions'],

isQuiet: true,

density: 'compact'

},

hmmmmm.... we don't havre this on any other component (other than ones designed for this, e.g. Text)? Are we sure this is needed? When would you set the slot to something other than the default? Also doesn't appear to be used by this PR?

Yeah, currently the default slot provided to ActionMenu/ActionGroup suffices for now, definitely isn't needed but was prompted by the ListView stories:

react-spectrum/packages/@react-spectrum/list/stories/ListView.stories.tsx

Lines 255 to 261 in 418e59b

<ActionGroup buttonLabelBehavior="hide" {...props} slot="actionGroup">

<Item key="info">

<Info />

<Text>Info</Text>

</Item>

</ActionGroup>

<ActionMenu {...props} slot="actionMenu">

. I'll remove it for now.

LFDanLu · 2022-05-05T22:55:00Z

packages/@react-spectrum/menu/src/ActionMenu.tsx

@@ -24,7 +24,7 @@ import {useMessageFormatter} from '@react-aria/i18n';
 import {useSlotProps} from '@react-spectrum/utils';

 function ActionMenu<T extends object>(props: SpectrumActionMenuProps<T>, ref: FocusableRef<HTMLButtonElement>) {
-  props = useSlotProps(props, 'actionmenu');
+  props = useSlotProps(props, 'actionMenu');


Brings it in line with the slot naming convention set up by ActionGroup, ButtonGroup etc. None of our other components used this slot except for ListView so we should be fine changing it here

fyi, this was the only reference i found in quarry for the 'actionmenu' slot https://git.corp.adobe.com/frontend/quarry/blob/9aa61397fad5f2b919d90bc67bad3c0fa5f3efbd/packages/%40quarry/inventory/src/views/list/ListViewItem.tsx#L103
you may want to double check me on that and make sure this is still safe

Hm, I don't believe this change will affect them, if anything that should already be broken if they are relying on getting our ListViewItem class from the actionMenu slot: https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/list/src/ListViewItem.tsx#L224

Oh never mind, that is a actionmenu grid area not a slot so the above change doesn't affect it at all. The actionmenu matches up with the ListView css grid area:

react-spectrum/packages/@react-spectrum/list/src/styles.css

Lines 230 to 231 in 8445498

"draghandle checkbox icon image content actions actionmenu chevron"

"draghandle checkbox icon image description actions actionmenu chevron"

reidbarber

LGTM

ktabors · 2022-05-06T20:33:33Z

Do we have a chromatic build of these stories to look at?

reidbarber · 2022-05-06T20:35:50Z

Do we have a chromatic build of these stories to look at?
@ktabors

Chromatic: https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=257
Storybook: https://5f0dd5ad2b5fc10022a2e320-xdfdzbotwk.chromatic.com/?path=/story/accordion--default

snowystinger · 2022-05-06T21:18:48Z

packages/@react-spectrum/list/chromatic/ListView.chromatic.tsx


 const meta: Meta = {
  title: 'ListView',
  component: ListView,
  parameters: {
+    chromaticProvider: {colorSchemes: ['light', 'dark'], locales: ['en-US'], scales: ['medium', 'large'], disableAnimations: true},


did it just extend too far to do lightest and darkest?

yeah, would mean more story splitting for performance. I'm beginning to think that perhaps we should have a separate story per combo, might be a lot though

Looking at storybookjs/storybook#9828, doesn't seem to be a great way to do this w/o going back to the storiesOf api. I'm leaning towards keeping this PR as is since we are looking to upgrade our storybook version and waiting for overflowMode to go in so we can get a relatively stable set of combinations per story (otherwise any changes to the power set will result in a difficult to comprehend diff).

snowystinger · 2022-05-06T21:19:24Z

packages/@react-spectrum/list/chromatic/ListViewRTL.chromatic.tsx

+
+export default meta;
+
+export {


nice, love this

snowystinger · 2022-05-06T21:20:15Z

packages/@react-types/actiongroup/src/index.d.ts

+  /**
+   * A slot to place the ActionGroup in.
+   * @default 'actionGroup'
+   */
+  slot?: string


makes sense, it should be here

ktabors

LGTM

ktabors · 2022-05-06T20:35:15Z

packages/@react-spectrum/list/chromatic/ListView.chromatic.tsx

-  {name: 'row 1'},
-  {name: 'row 2'},
-  {name: 'row 3'}
+// TODO: add overflow mode and thumbnails when those PRs are merged


should this be documented in github as an issue or project task instead of a todo?

sure, will add if this goes in before those PRs

ktabors · 2022-05-06T22:04:49Z

packages/@react-spectrum/list/chromatic/ListView.chromatic.tsx

+// TODO: add overflow mode and thumbnails when those PRs are merged
+let states = [
+  {isQuiet: true},
+  // No visual difference between single and multiple


Shouldn't we have a story with selectedKeys and then we'd need multiple selection to see how that looks?

The story already has selectedKeys with multiple selected, though this should probably be {selectionMode: 'multiple'}, rather than 'single. Unlike TableView where singleandmultiple` will either hide/display the select all checkbox, ListView doesn't have this distinction and thus we don't need to have both selection modes in the power set

ktabors · 2022-05-06T22:09:54Z

packages/@react-types/actiongroup/src/index.d.ts

+  /**
+   * A slot to place the ActionGroup in.
+   * @default 'actionGroup'
+   */
+  slot?: string


Where is this slot being used by ListView?

…chromatic

adobe-bot · 2022-05-09T23:19:04Z

Build successful! 🎉

adobe-bot · 2022-05-09T23:27:23Z

Build successful! 🎉

…chromatic

adobe-bot · 2022-05-10T17:42:12Z

Build successful! 🎉

adobe-bot · 2022-05-10T20:24:40Z

Build successful! 🎉

LFDanLu · 2022-05-10T20:28:56Z

latest Chromatic build: https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=261
Attempted to reorg into the following structure to support Darkest + Lightest:

export const DarkestMedium = Template().bind({});
DarkestMedium.storyName = 'all visual options, darkest, medium';
DarkestMedium.args = {combos: combinations};
DarkestMedium.parameters = {
  chromaticProvider: {
    ...defaultParams.chromaticProvider,
    colorSchemes: ['darkest'],
    scales: ['medium']
  }
};

export const DarkestLarge = Template().bind({});
DarkestLarge.storyName = 'all visual options, darkest, large';
DarkestLarge.args = {combos: combinations};
DarkestLarge.parameters = {
  chromaticProvider: {
    ...defaultParams.chromaticProvider,
    colorSchemes: ['darkest'],
    scales: ['large']
  }
};

but that was problematic w/ RTL support. Will need to refactor with changes to https://github.com/adobe/react-spectrum/blob/main/.chromatic/custom-addons/chromatic/index.js to properly support RTL + certain theme and size combos which may change a bunch of stories so will be followup

adobe-bot · 2022-05-10T23:30:23Z

Build successful! 🎉

…into listview_chromatic

adobe-bot · 2022-05-11T00:49:13Z

Build successful! 🎉

adobe-bot · 2022-05-11T00:50:10Z

Build successful! 🎉

adobe-bot · 2022-05-11T16:44:44Z

Build successful! 🎉

adding listview chromatic stories

1b4c449

LFDanLu commented May 5, 2022

View reviewed changes

reidbarber previously approved these changes May 6, 2022

View reviewed changes

snowystinger reviewed May 6, 2022

View reviewed changes

ktabors previously approved these changes May 6, 2022

View reviewed changes

removing comment and fixing selectionMode state

8de08d6

LFDanLu dismissed stale reviews from ktabors and reidbarber via 8de08d6 May 9, 2022 23:13

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

a375570

…chromatic

adding larger delay and thumbnail

2661f98

LFDanLu added 2 commits May 10, 2022 10:24

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

615a866

…chromatic

updating chromatic stories for overflow mode

e6ea88d

Merge branch 'main' into listview_chromatic

a077bfb

Merge branch 'main' into listview_chromatic

4786bad

snowystinger previously approved these changes May 10, 2022

View reviewed changes

LFDanLu added 2 commits May 10, 2022 17:40

removing slot props from ActionMenu and ActionGroup

cfb6357

Merge branch 'listview_chromatic' of github.com:adobe/react-spectrum …

6afde4b

…into listview_chromatic

LFDanLu dismissed snowystinger’s stale review via 6afde4b May 11, 2022 00:40

Merge branch 'main' into listview_chromatic

e412d96

dannify approved these changes May 11, 2022

View reviewed changes

Merge branch 'main' into listview_chromatic

2e3b3f1

reidbarber approved these changes May 11, 2022

View reviewed changes

LFDanLu merged commit 628791e into main May 11, 2022

LFDanLu deleted the listview_chromatic branch May 11, 2022 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ListView chromatic stories #3106

Adding ListView chromatic stories #3106

LFDanLu commented May 5, 2022 •

edited

Loading

adobe-bot commented May 5, 2022

LFDanLu May 5, 2022

snowystinger May 6, 2022

ktabors May 6, 2022

LFDanLu May 9, 2022

devongovett May 11, 2022 •

edited

Loading

LFDanLu May 11, 2022

LFDanLu May 5, 2022 •

edited

Loading

snowystinger May 10, 2022

LFDanLu May 10, 2022

LFDanLu May 10, 2022

reidbarber left a comment

ktabors commented May 6, 2022

reidbarber commented May 6, 2022 •

edited

Loading

snowystinger May 6, 2022

LFDanLu May 6, 2022

LFDanLu May 9, 2022

snowystinger May 6, 2022

snowystinger May 6, 2022

ktabors left a comment

ktabors May 6, 2022

LFDanLu May 9, 2022 •

edited

Loading

ktabors May 6, 2022

LFDanLu May 9, 2022

ktabors May 6, 2022

adobe-bot commented May 9, 2022

adobe-bot commented May 9, 2022

adobe-bot commented May 10, 2022

adobe-bot commented May 10, 2022

LFDanLu commented May 10, 2022

adobe-bot commented May 10, 2022

adobe-bot commented May 11, 2022

adobe-bot commented May 11, 2022

adobe-bot commented May 11, 2022

	actionGroup: {
	UNSAFE_className: listStyles['react-spectrum-ListViewItem-actions'],
	isQuiet: true,
	density: 'compact'
	},

	<ActionGroup buttonLabelBehavior="hide" {...props} slot="actionGroup">
	<Item key="info">
	<Info />
	<Text>Info</Text>
	</Item>
	</ActionGroup>
	<ActionMenu {...props} slot="actionMenu">

	"draghandle checkbox icon image content actions actionmenu chevron"
	"draghandle checkbox icon image description actions actionmenu chevron"

Adding ListView chromatic stories #3106

Adding ListView chromatic stories #3106

Conversation

LFDanLu commented May 5, 2022 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

adobe-bot commented May 5, 2022

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

devongovett May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LFDanLu May 5, 2022 • 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

reidbarber left a comment

Choose a reason for hiding this comment

ktabors commented May 6, 2022

reidbarber commented May 6, 2022 • 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

ktabors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LFDanLu May 9, 2022 • 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

adobe-bot commented May 9, 2022

adobe-bot commented May 9, 2022

adobe-bot commented May 10, 2022

adobe-bot commented May 10, 2022

LFDanLu commented May 10, 2022

adobe-bot commented May 10, 2022

adobe-bot commented May 11, 2022

adobe-bot commented May 11, 2022

adobe-bot commented May 11, 2022

LFDanLu commented May 5, 2022 •

edited

Loading

devongovett May 11, 2022 •

edited

Loading

LFDanLu May 5, 2022 •

edited

Loading

reidbarber commented May 6, 2022 •

edited

Loading

LFDanLu May 9, 2022 •

edited

Loading