Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding ListView chromatic stories #3106

Merged
merged 12 commits into from
May 11, 2022
Merged
135 changes: 105 additions & 30 deletions packages/@react-spectrum/list/chromatic/ListView.chromatic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,55 +10,124 @@
* governing permissions and limitations under the License.
*/

import {ActionButton} from '@react-spectrum/button';
import {ActionGroup} from '@react-spectrum/actiongroup';
import {ActionMenu} from '@react-spectrum/menu';
import Add from '@spectrum-icons/workflow/Add';
import {Content, View} from '@react-spectrum/view';
import {Flex} from '@react-spectrum/layout';
import {Heading} from '@react-spectrum/text';
import Delete from '@spectrum-icons/workflow/Delete';
import Folder from '@spectrum-icons/workflow/Folder';
import {generatePowerset} from '@react-spectrum/story-utils';
import {Grid, repeat} from '@react-spectrum/layout';
import {Heading, Text} from '@react-spectrum/text';
import {IllustratedMessage} from '@react-spectrum/illustratedmessage';
import Info from '@spectrum-icons/workflow/Info';
import {Item, ListView} from '../';
import {Link} from '@react-spectrum/link';
import {Meta, Story} from '@storybook/react';
import React from 'react';

let flatOptions = [
{name: 'row 1'},
{name: 'row 2'},
{name: 'row 3'}
// TODO: add overflow mode and thumbnails when those PRs are merged
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@LFDanLu LFDanLu May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, will add if this goes in before those PRs

let states = [
{isQuiet: true},
// No visual difference between single and multiple
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{selectionMode: 'single'},
{density: ['compact', 'spacious']},
{selectionStyle: 'highlight'}
];

let withButtons = [
{name: 'row 1', button: 'Button 1'},
{name: 'row 2', button: 'Button 2'},
{name: 'row 3', button: 'Button 3'}
];
let combinations = generatePowerset(states);
let chunkSize = Math.ceil(combinations.length / 3);

function shortName(key, value) {
let returnVal = '';
switch (key) {
case 'isQuiet':
returnVal = 'quiet';
break;
case 'selectionMode':
returnVal = `sm: ${value === undefined ? 'none' : value}`;
break;
case 'density':
returnVal = `den: ${value === undefined ? 'regular' : value}`;
break;
case 'selectionStyle':
returnVal = 'highlight';
break;
}
return returnVal;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// noticed a small shifting before final layout, delaying so chromatic doesn't hit that
chromatic: {delay: 600}
}
};

export default meta;

const Template = (): Story => (args) => (
<ListView {...args} width="size-3400" items={flatOptions}>
{(item) => <Item key={item.name}>{item.name}</Item>}
</ListView>
const renderActions = (
<>
<ActionGroup buttonLabelBehavior="hide">
<Item key="info">
<Info />
<Text>Info</Text>
</Item>
</ActionGroup>
<ActionMenu>
<Item key="add">
<Add />
<Text>Add</Text>
</Item>
<Item key="delete">
<Delete />
<Text>Delete</Text>
</Item>
</ActionMenu>
</>
);

const TemplateWithButtons = (): Story => (args) => (
<ListView {...args} width="size-3400" items={withButtons}>
{(item) => (
<Item key={item.name}>
<Flex alignItems="center">
<View flexGrow={1}>{item.name}</View>
<ActionButton>{item.button}</ActionButton>
</Flex>
</Item>
)}
</ListView>
const Template = (): Story => ({combos, ...args}) => (
<Grid columns={repeat(3, '1fr')} autoFlow="row" gap="size-300">
{combos.map(c => {
let key = Object.keys(c).map(k => shortName(k, c[k])).join(' ');
if (!key) {
key = 'empty';
}
return (
<View flexGrow={1} maxWidth="size-5000" maxHeight={700} id={key}>
<ListView aria-label={key} width="size-3600" height="100%" selectedKeys={['a', 'd']} disabledKeys={['c']} {...args} {...c}>
<Item key="a" textValue="Utilities" hasChildItems>
<Folder />
<Content>
<Link>Utilities</Link>
</Content>
<Text slot="description">16 items</Text>
{renderActions}
</Item>
<Item key="b" textValue="Adobe Photoshop">
<Content>Adobe Photoshop</Content>
<Text slot="description">Application</Text>
{renderActions}
</Item>
<Item key="c" textValue="Adobe Illustrator">
<Content>Adobe Illustrator</Content>
<Text slot="description">Application</Text>
{renderActions}
</Item>
<Item key="d" textValue="Adobe XD">
<Content>Adobe XD</Content>
<Text slot="description">Application</Text>
{renderActions}
</Item>
</ListView>
</View>
);
})}
</Grid>
);

function renderEmptyState() {
Expand All @@ -80,10 +149,16 @@ const TemplateEmptyState = (): Story => () => (
);

export const Default = Template().bind({});
Default.storyName = 'default';
Default.storyName = 'all visual option combos 1 of 3';
Default.args = {combos: combinations.slice(0, chunkSize)};

export const ComboPt2 = Template().bind({});
ComboPt2.args = {combos: combinations.slice(chunkSize, chunkSize * 2)};
ComboPt2.storyName = 'all visual option combos 2 of 3';

export const WithButtons = TemplateWithButtons().bind({});
WithButtons.storyName = 'with buttons';
export const ComboPt3 = Template().bind({});
ComboPt3.args = {combos: combinations.slice(chunkSize * 2, chunkSize * 3)};
ComboPt3.storyName = 'all visual option combos 3 of 3';

export const Empty = TemplateEmptyState().bind({});
Empty.storyName = 'empty state';
30 changes: 30 additions & 0 deletions packages/@react-spectrum/list/chromatic/ListViewRTL.chromatic.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 2022 Adobe. All rights reserved.
* This file is licensed to you under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. You may obtain a copy
* of the License at http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under
* the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
* OF ANY KIND, either express or implied. See the License for the specific language
* governing permissions and limitations under the License.
*/

import {Meta} from '@storybook/react';

const meta: Meta = {
title: 'ListViewRTL',
parameters: {
chromaticProvider: {colorSchemes: ['light', 'dark'], locales: ['ar-AE'], scales: ['medium', 'large'], disableAnimations: true},
chromatic: {delay: 600}
}
};

export default meta;

export {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, love this

Default,
ComboPt2,
ComboPt3,
Empty
} from './ListView.chromatic';
2 changes: 1 addition & 1 deletion packages/@react-spectrum/menu/src/ActionMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member Author

@LFDanLu LFDanLu May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

let formatMessage = useMessageFormatter(intlMessages);
let buttonProps = filterDOMProps(props, {labelable: true});
if (buttonProps['aria-label'] === undefined) {
Expand Down
7 changes: 6 additions & 1 deletion packages/@react-types/actiongroup/src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,10 @@ export interface SpectrumActionGroupProps<T> extends AriaActionGroupProps<T>, St
*/
buttonLabelBehavior?: 'show' | 'collapse' | 'hide',
/** The icon displayed in the dropdown menu button when a selectable ActionGroup is collapsed. */
summaryIcon?: ReactElement
summaryIcon?: ReactElement,
/**
* A slot to place the ActionGroup in.
* @default 'actionGroup'
*/
slot?: string
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, it should be here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this slot being used by ListView?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This slot is used here:

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

Copy link
Member

@devongovett devongovett May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<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.

}
7 changes: 6 additions & 1 deletion packages/@react-types/menu/src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,5 +86,10 @@ export interface SpectrumActionMenuProps<T> extends CollectionBase<T>, MenuTrigg
/** Whether the element should receive focus on render. */
autoFocus?: boolean,
/** Handler that is called when an item is selected. */
onAction?: (key: Key) => void
onAction?: (key: Key) => void,
/**
* A slot to place the action menu in.
* @default 'actionMenu'
*/
slot?: string
}