Skip to content

feat(react-tags): replace TagButton with Tag interactive prop#27794

Closed
YuanboXue-Amber wants to merge 21 commits into
microsoft:masterfrom
YuanboXue-Amber:tag-content
Closed

feat(react-tags): replace TagButton with Tag interactive prop#27794
YuanboXue-Amber wants to merge 21 commits into
microsoft:masterfrom
YuanboXue-Amber:tag-content

Conversation

@YuanboXue-Amber

@YuanboXue-Amber YuanboXue-Amber commented May 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR updates api for @fluentui/react-tags:

Interactive tag was a separate control TagButton. Now it is a prop interactive on Tag component. When interactive=true, Tag content slot renders button instead of div.

Before:

image 
// tag with a dismiss button
<Tag dismissible>
  Primary text
</Tag>


// INTERACTIVE tag with a dismiss button
<TagButton dismissible>
  Primary text
</TagButton>

After

No change in UI.

// tag with a dismiss button
<Tag dismissible>
  Primary text
</Tag>


// INTERACTIVE tag with a dismiss button
<Tag interactive dismissible>
  Primary text
</Tag>

@fabricteam

fabricteam commented May 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-tags
Tag		 21.687 kB
7.868 kB		 24.907 kB
8.319 kB		 3.22 kB
451 B

🤖 This report was generated against 45628014c675548294ab93a4a8bc66dae8eeb7ba

@YuanboXue-Amber YuanboXue-Amber marked this pull request as ready for review May 9, 2023 06:25
@YuanboXue-Amber YuanboXue-Amber requested a review from a team as a code owner May 9, 2023 06:25
@codesandbox-ci

codesandbox-ci Bot commented May 9, 2023 •
edited
Loading

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 51bcf58:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor

size-auditor Bot commented May 9, 2023 •
edited
Loading

Copy link
Copy Markdown

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 45628014c675548294ab93a4a8bc66dae8eeb7ba (build)

@microsoft microsoft deleted a comment from azure-pipelines Bot May 9, 2023
ling1726
ling1726 reviewed May 9, 2023
View reviewed changes
Comment thread packages/react-components/react-tags/src/components/TagContent/useTagContent.ts Outdated
ling1726
ling1726 reviewed May 9, 2023
View reviewed changes
Comment thread packages/react-components/react-tags/src/components/TagContent/useTagContent.ts Outdated
ling1726
ling1726 reviewed May 9, 2023
View reviewed changes
Comment thread packages/react-components/react-tags/src/components/Tag/TagContext.ts Outdated
ling1726
ling1726 reviewed May 9, 2023
View reviewed changes
Comment thread packages/react-components/react-tags/stories/Tag/TagDefault.stories.tsx Outdated
@YuanboXue-Amber YuanboXue-Amber marked this pull request as draft May 9, 2023 08:41
@YuanboXue-Amber YuanboXue-Amber marked this pull request as ready for review May 9, 2023 10:10
@YuanboXue-Amber YuanboXue-Amber requested a review from a team as a code owner May 9, 2023 10:10
@YuanboXue-Amber YuanboXue-Amber changed the title feat(react-tags): replace TagButton with Tag interactive prop; replace content slot with TagContent feat(react-tags): replace TagButton with Tag interactive prop May 9, 2023
bsunderhus
bsunderhus approved these changes May 9, 2023
View reviewed changes

@bsunderhus bsunderhus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGM, some minor issues on the slots types declaration but nothing major.

A good rule of thumbs is:

  1. if the slot is required, then:
    • make it non optional on TagSlot definiton (remove ?)
    • make it NonNullable
    • set required:true in resolveShorthand invocation
  2. if the slot is not required then add ? to ensure the render methods will follow the type.

Comment thread packages/react-components/react-tags/src/components/Tag/Tag.types.ts
Comment thread packages/react-components/react-tags/src/components/Tag/renderTag.tsx
@bsunderhus bsunderhus self-requested a review May 9, 2023 12:55
@YuanboXue-Amber YuanboXue-Amber mentioned this pull request May 16, 2023
Merged
@YuanboXue-Amber

YuanboXue-Amber commented May 16, 2023

Copy link
Copy Markdown
Contributor Author

close due to #27858

@YuanboXue-Amber YuanboXue-Amber deleted the tag-content branch May 16, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bsunderhus bsunderhus bsunderhus approved these changes
+1 more reviewer
@ling1726 ling1726 ling1726 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tomi-msft tomi-msft

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@YuanboXue-Amber @fabricteam @bsunderhus @ling1726 @tomi-msft