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

docs(react-infobutton): Adding component's spec #25251

Merged
merged 6 commits into from
Nov 2, 2022

Conversation

sopranopillow
Copy link
Contributor

@sopranopillow sopranopillow commented Oct 14, 2022

This PR adds the spec for InfoButton. Note: this is still a WIP and will be updated after a design review of the component.

Preview

Related Issue(s)

Fixes #25061

@fabricteam
Copy link
Collaborator

fabricteam commented Oct 14, 2022

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-infobutton
InfoButton
7.742 kB
3.265 kB
🤖 This report was generated against ea1842baec1d31d0936665bdffedf5803049eb46

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 14, 2022

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 a36ef96:

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

@size-auditor
Copy link

size-auditor bot commented Oct 20, 2022

Asset size changes

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

Baseline commit: ea1842baec1d31d0936665bdffedf5803049eb46 (build)


- _Migration from v8_
- _Migration from v0_
There's no migration guide as `v0` and `v8` do not have an InfoButton component.
Copy link
Contributor

@kolaps33 kolaps33 Oct 24, 2022

Choose a reason for hiding this comment

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

Hello :)
there was no similar component in v0 or in 'v8' but one version exists in the TMP repository(in MS Teams code)

Implementation accessibility details are following:

This approach was approved by accessibility PM for MS Teams(Brett Humphrey).

If there is a button used, then generally screen reader users would expect there is some action available on the button.

I believe we should be aligned together with v9 and MS Teams component.

@layershifter
Copy link
Member

@sopranopillow I was thinking about this spec and here is a proposal from my side...

Make InfoButton a button

In the spec currently button is a slot at InfoButton component. It looks a bit strange as even our form components have a concept our primary slot and behave differently. The only components that are custom are hosts (Menu, Popover, Tooltip, etc.) and triggers (MenuTrigger, etc.). However, both hosts and triggers don't render DOM elements.

So, what is a proposed change? In terms of render template:

  <Popover>
    <PopoverTrigger>
      <slots.root {...slotProps.root} />
    </PopoverTrigger>
    <PopoverContent />
  </Popover>

Instead of Popover being a root - the proposal is to have a button as a root (this solves this concern #25251 (comment)).

This change allows following:

  • Use Tooltips and other components:
      <Tooltip content="A tooltip">
         <InfoButton>
           A button
         </InfoButton>
       </Tooltip>
  • Use props directly (I mean className, style, ref and etc.)
      <InfoButton className='foo' ref={buttonRef} />
      // over
     <InfoButton className='foo' ref={buttonRef} /> // 💥 does not work
     <InfoButton button={{ className: 'foo', ref: buttonRef }} /> // 😕 works, but not consistent with other components
  • We can pass children to a component that makes usages intuitive
    <InfoButton>Content goes here</InfoButton>

What to do with Popover then?

It's a good question for that proposal 👆

We don't need to configure PopoverTrigger, but we need to provide an access to Popover and PopoverSurface. I was thinking about popover & popoverSurface slots:

<InfoButton popover={{ open, positioning }} popoverSurface={{ children: 'Content' }}>
   A button
</InfoButton>
  • where popover is an object with slots and cannot have shorthand form and children
  • where popoverSurface is a regular slot

Another option is combine that with a current proposal and keep Popover props directly on InfoButton:

<InfoButton open, positioning={{ /* ... */ }} popoverSurface={{ children: 'Content' }}>
   A button
</InfoButton>

From my POV it fits more into v9 API design, WDYT?

@sopranopillow sopranopillow marked this pull request as ready for review October 27, 2022 17:19
@sopranopillow
Copy link
Contributor Author

sopranopillow commented Oct 27, 2022

@sopranopillow I was thinking about this spec and here is a proposal from my side...

Make InfoButton a button

In the spec currently button is a slot at InfoButton component. It looks a bit strange as even our form components have a concept our primary slot and behave differently. The only components that are custom are hosts (Menu, Popover, Tooltip, etc.) and triggers (MenuTrigger, etc.). However, both hosts and triggers don't render DOM elements.

...

From my POV it fits more into v9 API design, WDYT?

@layershifter Love it! I think this is a great candidate for a recipe on how to create a PopupButton (name not set) the Fluent v9 way!


### Anatomy

![Anatomy](./etc/images/anatomy.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

This image shows the popover in the "above-start" position. Will InfoButton support positioning of the popover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that will be done through the root as it takes the popover props as well. The updated types now have the Popover props.

packages/react-components/react-infobutton/Spec.md Outdated Show resolved Hide resolved
packages/react-components/react-infobutton/Spec.md Outdated Show resolved Hide resolved
packages/react-components/react-infobutton/Spec.md Outdated Show resolved Hide resolved
- Identify UI parts that appear on **hover or focus** and specify keyboard and screen reader interaction with them
- List cases when **focus** needs to be **trapped** in sections of the UI (for dialogs and popups or for hierarchical navigation)
- List cases when **focus** needs to be **moved programatically** (if parts of the UI are appearing/disappearing or other cases)
- `role="tooltip"` is used on the PopoverSurface.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we then also need to use aria-describedby or aria-labeledby on the button itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't set yet, once we iron it out, I'll update the spec.

@sopranopillow sopranopillow merged commit 3d82854 into microsoft:master Nov 2, 2022
@sopranopillow sopranopillow deleted the info-button/spec branch November 2, 2022 22:00
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Nov 3, 2022
* master:
  feat(scripts): improve API violation reporting (microsoft#25356)
  applying package updates
  fix: Preventing blanket selectors from Fabric component from being applied via new preventBlanketFontInheritance prop (microsoft#25453)
  feat(react-infobutton): Add initial implementation (microsoft#25247)
  chore(react-avatar, avatar-context): migrate to new package structure (microsoft#25473)
  chore(react-portal, portal-compat, portal-compat-context): migrate to new package structure (microsoft#25481)
  docs(react-infobutton): Adding component's spec (microsoft#25251)
  fix(screener-build workflow): scope package to build for v9 VR tests to speed up perf (microsoft#25494)
  chore(vr-tests-v9): Convert Button VR tests to CSF (microsoft#25108)
@sopranopillow sopranopillow mentioned this pull request Nov 8, 2022
30 tasks
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
* restoring work

* adding requested changes

* adding requested changes

* adding requested changes

* adding requested changes

* adding requested changes
Hotell pushed a commit to Hotell/fluentui that referenced this pull request Feb 9, 2023
* restoring work

* adding requested changes

* adding requested changes

* adding requested changes

* adding requested changes

* adding requested changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InfoButton: Write and merge spec
8 participants