RSP-1129: Tag and TagGroup component #10

milanbrkic-ms · 2019-09-26T18:15:08Z

Closes

✅ Pull Request Checklist:

Included link to corresponding 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 exist for this component).
Looked at the Accessibility Standard and/or talked to @mijordan about Accessibility for this feature.

📝 Test Instructions:

🧢 Your Team:

github-actions · 2019-09-26T18:17:36Z

Build successful! View the storybook

github-actions · 2019-09-30T09:18:35Z

Build successful! View the storybook

packages/@react-aria/tag/src/useTagGroup.ts

packages/@react-aria/tag/src/useTag.ts

packages/@react-aria/tag/src/useTagGroup.ts

github-actions · 2019-10-02T15:39:50Z

Build successful! View the storybook

github-actions · 2019-10-02T16:06:22Z

Build successful! View the storybook

github-actions · 2019-10-07T11:29:26Z

Build successful! View the storybook

snowystinger

CSS review

packages/@adobe/spectrum-css-temp/components/tags/index.css

snowystinger · 2019-10-07T20:55:24Z

packages/@adobe/spectrum-css-temp/components/tags/skin.css

+    .spectrum-Tags-item-ClearButton {
+      color: var(--spectrum-tag-border-color-error);
+      &.clearButton-focus {
+         color: white;


should probably be a var, i don't know which one though, @lazd can you review?

Removed this line - new designs 🎈

snowystinger · 2019-10-07T20:55:42Z

packages/@adobe/spectrum-css-temp/components/tags/skin.css

+  > .spectrum-Tags-item-ClearButton {
+    &.clearButton-focus {
+      background-color: var(--spectrum-tag-deletable-background-color-focus);
+      color: white;


Please use DNA tokens for these colors. Probably one of these:

--spectrum-tag-removable-icon-color: var(--spectrum-global-color-gray-600); --spectrum-tag-removable-icon-color-hover: var(--spectrum-global-color-gray-900); --spectrum-tag-removable-icon-color-down: var(--spectrum-global-color-gray-900); --spectrum-tag-removable-icon-color-key-focus: var(--spectrum-global-color-gray-900); --spectrum-tag-removable-icon-color-disabled: var(--spectrum-global-color-gray-500); --spectrum-tag-removable-icon-color-error: var(--spectrum-global-color-red-500); --spectrum-tag-removable-icon-color-error-hover: var(--spectrum-global-color-red-600); --spectrum-tag-removable-icon-color-error-key-focus: var(--spectrum-global-color-red-600); --spectrum-tag-removable-icon-color-error-down: var(--spectrum-global-color-red-700); --spectrum-tag-removable-icon-color-error-disabled: var(--spectrum-global-color-gray-500);

If these tokens are out of date, please send a PR to Spectrum DNA: https://git.corp.adobe.com/Spectrum/spectrum-dna

packages/@adobe/spectrum-css-temp/components/tags/skin.css

packages/@adobe/spectrum-css-temp/vars/spectrum-light.css

snowystinger · 2019-10-07T21:03:16Z

packages/@react-spectrum/tag/src/Tag.tsx

+import {useTag} from '@react-aria/tag';
+
+export interface TagProps extends DOMProps {
+  children?: string | ReactNode,


does ReactElement cover this case?

Yes it does 👍

unresolving until it's done

packages/@react-spectrum/tag/src/Tag.tsx

packages/@react-aria/tag/src/useTag.ts

packages/@react-spectrum/tag/test/Tag.test.js

milanbrkic-ms · 2019-10-08T15:09:58Z

Few notes @snowystinger :

I tried to implement keyboard selection for TagGroup, but ArrowLeft wasn't working. I could commit if you want to see changes or to make follow up task for that one?
onRemove is going to be called with children value. Are we fine with that? What if children is React component itself? Maybe to return value prop for tag component so we could haveonRemove(value || children)?

github-actions · 2019-10-08T15:14:49Z

Build successful! View the storybook

packages/@react-aria/tag/src/useTag.ts

packages/@react-aria/tag/intl/en-US.json

packages/@react-aria/tag/src/useTagGroup.ts

LFDanLu · 2019-10-08T16:58:32Z

packages/@react-aria/tag/test/useTag.test.js

+    let {result} = renderHook(() => useTag(props));
+    expect(result.current.tagProps.onKeyDown).toBe(null);
+    expect(result.current.tagProps.tabIndex).toBe(-1);
+  });


Will there be tests for the clearButtonProps as well or is that a later TODO?

github-actions · 2019-10-09T17:29:31Z

Build successful! View the storybook

github-actions · 2019-10-11T15:49:17Z

Build successful! View the storybook

packages/@adobe/spectrum-css-temp/components/tags/index.css

devongovett · 2019-10-16T16:13:43Z

packages/@adobe/spectrum-css-temp/vars/spectrum-dark.css

  --spectrum-tag-border-color-error: rgb(236, 91, 98);
  --spectrum-tag-icon-color-error: rgb(236, 91, 98);
-  --spectrum-tag-text-color-error: rgb(185, 185, 185);
+  --spectrum-tag-text-color-error: rgb(247, 109, 116);


we'll need to contribute these variable changes to spectrum-dna at some point

Yes. This is a generated file that will be overwritten. Do not edit it.

What is procedure?
Open pr in spectrum-dna and remove css changes from this PR and then this pr needs to wait spectrum-dna pr to be merged in order to have latest changes? Is it same if I need to add new variable? For example I used global var here https://github.com/adobe/react-spectrum-v3/pull/10/files#diff-aa7d70936a7ea3560aa2240e8e869fa5R106 and I suppose it should be --spectrum-tag-something, right?

You can update/add the variables from your DNA PR in here temporarily, and we'll regenerate it later.

packages/@react-aria/tag/src/useTag.ts

packages/@react-aria/tag/src/useTagGroup.ts

devongovett · 2019-10-16T16:22:02Z

packages/@react-aria/tag/src/useTag.ts

+    onRemove,
+    children,
+    selected,
+    role


should we have a default role in case tag is used outside a tag group somehow?

packages/@react-spectrum/tag/src/Tag.tsx

packages/@react-aria/tag/src/useTag.ts

devongovett · 2019-10-16T16:25:22Z

FYI, we talked with design and they changed the focus style of the clear button again (sorry).

packages/@react-spectrum/tag/src/TagGroup.tsx

github-actions · 2019-10-18T10:28:21Z

Build successful! View the storybook

github-actions · 2019-10-18T10:41:39Z

Build successful! View the storybook

github-actions · 2019-10-18T14:19:53Z

Build successful! View the storybook

github-actions · 2019-10-21T17:07:24Z

Build successful! View the storybook

milanbrkic-ms · 2019-10-22T14:26:44Z

I will update css variables after https://git.corp.adobe.com/Spectrum/spectrum-dna/pull/338 is merged

github-actions · 2019-10-22T15:06:42Z

Build successful! View the storybook

github-actions · 2019-10-23T11:16:50Z

Build successful! View the storybook

packages/@react-aria/tag/src/useTag.ts

milanbrkic-ms · 2019-10-23T11:51:03Z

packages/@react-spectrum/tag/stories/TagGroup.stories.tsx

      </TagGroup>
    )
+  ).add(
+    'with announcing',


In order to have announcing you need to navigate with keyboard into first tag and then you can press ctrl+d in order to create new tag. In that moment announcing should be triggered.

packages/@react-aria/tag/src/useTagGroup.ts

packages/@react-aria/tag/src/useTag.ts

packages/@react-aria/tag/src/useTagGroup.ts

ktabors · 2019-10-24T00:32:12Z

packages/@react-aria/tag/src/useTag.ts

+      id: buttonId,
+      title: removeString,
+      isDisabled,
+      role


It's weird to me that the labelProps and the clearButtonProps have the same role. I'd expect the role only to be on the tagProps.

To be honest I am new to accessibility and nothing makes sense, but here is the comment where Michael explains why #10 (comment)

Within a TagGroup, we are implementing the WAI-ARIA 1.1 Layout Grid design pattern: https://www.w3.org/TR/wai-aria-practices/examples/grid/LayoutGrids.html#ex2_label. The TagGroup container element has role="grid". Each Tag is a row within the grid with role="row". Each Tag contains one or, if the Tag can be removed, two cells. The label will have role="gridcell", and a wrapper span element around the clear button will also have role="gridcell".

ktabors · 2019-10-24T00:34:11Z

packages/@react-aria/tag/src/useTagGroup.ts

+  tagGroupProps: HTMLAttributes<HTMLElement>
+}
+
+export function useTagGroup(props: AriaTagGroupProps): TagGroupAria {


Should useTagGroup have tests?

It is returning plain object, there is noting to test. But if you feel it should have, I can add

ktabors · 2019-10-24T02:53:20Z

packages/@react-aria/tag/test/useTag.test.js

+import {renderHook} from 'react-hooks-testing-library';
+import {useTag} from '../';
+
+describe('useTag tests', () => {


should 'aria-selected' be tested too?

packages/@react-spectrum/tag/src/TagGroup.tsx

ktabors · 2019-10-24T02:59:44Z

packages/@react-spectrum/tag/src/Tag.tsx

+    role
+  } =  useTagGroupProvider();
+
+  let removable = isRemovable !== undefined ? isRemovable : isGroupRemovable;


Why doesn't the group property take precedence?

I thought we should be flexible and allow user to override flags for child elements. I am not sure if there is a need for this, but it could be useful and it doesn't breaks anything. What do you think?

packages/@react-spectrum/tag/test/Tag.test.js

ktabors · 2019-10-24T03:20:04Z

packages/@react-spectrum/tag/src/Tag.tsx

+import {useTag} from '@react-aria/tag';
+import {useTagGroupProvider} from './TagGroup';
+
+export const Tag = ((props: TagProps) => {


forwardRef?

Do we need to forward ref? And why?

ktabors · 2019-10-24T03:20:11Z

packages/@react-spectrum/tag/src/TagGroup.tsx

+  return useContext(TagGroupContext);
+}
+
+export const TagGroup = ((props: TagGroupProps) => {


forwardRef?

ktabors · 2019-10-24T03:20:40Z

packages/@react-spectrum/tag/test/Tag.test.js

+import {Tag as V2Tag} from '@react/react-spectrum/TagList';
+
+
+describe('Tag', function () {


are more tests planned?

I've added more tests

ktabors · 2019-10-24T03:20:48Z

packages/@react-spectrum/tag/test/TagGroup.test.js

+import React from 'react';
+import {Tag, TagGroup} from '../src';
+
+describe('TagGroup', function () {


are more tests planned?

I've added more tests

github-actions · 2019-10-24T16:49:43Z

Build successful! View the storybook

devongovett

Looks good. You can do the following in a followup if you like.

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

devongovett · 2019-10-25T00:57:06Z

packages/@react-types/shared/src/removable.d.ts

+import {SyntheticEvent} from 'react';
+
+export interface Removable<T, R> {
+  isRemovable?: boolean,


hmm - this doesn't exist in the API spec for TagGroup. we have isReadOnly to mean this

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

devongovett · 2019-10-25T01:01:47Z

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

+  children?: ReactChild,
+  icon?: ReactElement,
+  isDisabled?: boolean,
+  isSelected?: boolean,


Shouldn't be a prop of Tag - should come from context. The selection state comes from MultipleSelectionBase on the group

I removed flag, but now I don't know how to handle selection until I have multi selection hook. I removed stories for selected Tag and overall usage. If you agree, we can implement once we have actual selection state. Maybe as part of https://jira.corp.adobe.com/browse/RSP-1357?

* AriaTagProps moved to @react-aria/tag package * Deleted isRemovable flag and its usage from TagGroup * Removed isSelected flag and its usage for Tag (stories removed)

…1129

github-actions · 2019-10-29T11:33:50Z

Build successful! View the storybook

github-actions · 2019-10-29T11:40:15Z

Build successful! View the storybook

github-actions · 2019-10-29T18:12:43Z

Build successful! View the storybook

github-actions · 2019-10-29T21:54:32Z

Build successful! View the storybook

milanbrkic-ms requested review from dannify, devongovett and snowystinger as code owners September 26, 2019 18:15

milanbrkic-ms changed the title ~~Rsp 1129~~ RSP-1129: Tag and TagGroup component Sep 26, 2019

snowystinger reviewed Oct 1, 2019

View reviewed changes

packages/@react-aria/tag/src/useTagGroup.ts Outdated Show resolved Hide resolved

packages/@react-aria/tag/src/useTag.ts Show resolved Hide resolved

packages/@react-aria/tag/src/useTagGroup.ts Outdated Show resolved Hide resolved

milanbrkic-ms requested a review from snowystinger October 7, 2019 12:52

snowystinger reviewed Oct 7, 2019

View reviewed changes

snowystinger reviewed Oct 8, 2019

View reviewed changes

packages/@react-aria/tag/src/useTag.ts Outdated Show resolved Hide resolved

LFDanLu requested changes Oct 8, 2019

View reviewed changes

snowystinger mentioned this pull request Oct 10, 2019

RSP-1127|RSP-1128: ProgressCircle and ProgressBar #9

Merged

5 tasks

devongovett reviewed Oct 16, 2019

View reviewed changes

majornista reviewed Oct 16, 2019

View reviewed changes

packages/@react-spectrum/tag/src/TagGroup.tsx Outdated Show resolved Hide resolved

RSP-1129: Tag and TagGroup component

e9eabff

milanbrkic-ms force-pushed the RSP-1129 branch from 7601b84 to e9eabff Compare October 18, 2019 10:35

addressing css comments

28c8a7c

one with improved accessibility, without announcing

e0afae4

implemented selected Tag state

352e469

one with stories for selected tag and css for more cases

9dcc7ca

One with announcing

6fbc48f

milanbrkic-ms commented Oct 23, 2019

View reviewed changes

devongovett requested changes Oct 24, 2019

View reviewed changes

packages/@react-aria/tag/src/useTagGroup.ts Outdated Show resolved Hide resolved

packages/@react-aria/tag/src/useTag.ts Outdated Show resolved Hide resolved

packages/@react-aria/tag/src/useTagGroup.ts Outdated Show resolved Hide resolved

ktabors reviewed Oct 24, 2019

View reviewed changes

Milan Brkic added 3 commits October 24, 2019 14:10

Use aria-live and removed TODO's

1b0748d

isReadOnly flag + tests

61e9d52

more tests

0c5e1a4

devongovett previously approved these changes Oct 28, 2019

View reviewed changes

Milan Brkic added 2 commits October 29, 2019 12:17

added dna variables into css file and addressed code review comments

94c463d

* AriaTagProps moved to @react-aria/tag package * Deleted isRemovable flag and its usage from TagGroup * Removed isSelected flag and its usage for Tag (stories removed)

Merge branch 'master' of github.com:adobe/react-spectrum-v3 into RSP-…

a64b1e0

…1129

milanbrkic-ms dismissed devongovett’s stale review via a64b1e0 October 29, 2019 11:27

revert spectrum-css-temp/vars files

ad98d7e

pasting vars from dna pr

2800457

devongovett approved these changes Oct 29, 2019

View reviewed changes

devongovett merged commit 74c68aa into master Oct 29, 2019

devongovett deleted the RSP-1129 branch October 29, 2019 21:49

devongovett added a commit that referenced this pull request Jul 25, 2024

Implement more ActionButton and ToggleButton variants (#10)

7567377

		import {Tag as V2Tag} from '@react/react-spectrum/TagList';


		describe('Tag', function () {

RSP-1129: Tag and TagGroup component #10

RSP-1129: Tag and TagGroup component #10

Uh oh!

Conversation

milanbrkic-ms commented Sep 26, 2019

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Team:

Uh oh!

github-actions bot commented Sep 26, 2019

Uh oh!

github-actions bot commented Sep 30, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 2, 2019

Uh oh!

github-actions bot commented Oct 2, 2019

Uh oh!

github-actions bot commented Oct 7, 2019

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

milanbrkic-ms commented Oct 8, 2019

Uh oh!

github-actions bot commented Oct 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 9, 2019

Uh oh!

github-actions bot commented Oct 11, 2019

Uh oh!

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!

Uh oh!