Add Progress SPEC #24418

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.

Sign up for GitHub

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

Jump to bottom

Merged

tomi-msft merged 5 commits into microsoft:master from tomi-msft:add-progress-spec

Oct 4, 2022

Contributor

tomi-msft commented Aug 18, 2022 •

edited

Loading

This PR is to add the initial Progress SPEC to the react-progress package. There might be some style changes or prop additions after a design review, and this initial SPEC will get updated to reflect that.

Fixes #24388


          Add Progress SPEC

3bdae90

tomi-msft requested a review from a team as a code owner

August 18, 2022 18:38

msft-fluent-ui-bot assigned layershifter

github-actions bot added this to the July Project Cycle Q3 2022 milestone

Collaborator

fabricteam commented Aug 18, 2022 •

edited

Loading

📊 Bundle size report

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-progress` Progress	`13.44 kB` `5.247 kB`

_{🤖 This report was generated against b0b7f99bbbb05b0bd136432983a74fea252c7163}

codesandbox-ci bot commented Aug 18, 2022 •

edited

Loading

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 9a2153f:

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

size-auditor bot commented Aug 18, 2022 •

edited

Loading

Asset size changes

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

Baseline commit: b0b7f99bbbb05b0bd136432983a74fea252c7163 (build)

layershifter reviewed

View reviewed changes

packages/react-components/react-progress/Spec.md

    
              ## Prior Art

              _Include background research done for this component_

              ### Open UI

Member

layershifter Aug 24, 2022

I don't see a reference there to OpenUI, is the title correct?

Member

khmakoto Sep 29, 2022

@tomi-msft you can add a link to Open UI if there is a page about Progress there. If a page for progress does not exist feel free to rename this section to something like "Comparison between other libraries".

packages/react-components/react-progress/Spec.md Outdated

Comment on lines 11 to 14

    
              | Library    | Component Name | Spec Link                                                          | Notes                                                                   |

              | ---------- | -------------- | ------------------------------------------------------------------ | ----------------------------------------------------------------------- |

              | Ant Design | Progress       | [Progress](https://ant.design/components/progress/)                | Specifies the type, a combined progress component                       |

              | Bootstrap  | Progress       | [Progress](https://getbootstrap.com/docs/4.3/components/progress/) | Allows for multiple different animation styles on the same Progress bar |

Member

layershifter Aug 24, 2022

I am surprised with such small list of prior art as this component is quite popular:

Did you consider other libraries in a research?

packages/react-components/react-progress/Spec.md Outdated Show resolved Hide resolved

packages/react-components/react-progress/Spec.md Outdated Show resolved Hide resolved

packages/react-components/react-progress/Spec.md Outdated Show resolved Hide resolved

packages/react-components/react-progress/Spec.md

    
              ### Props

              See API at [Progress.types.tsx](./src/components/Progress/Progress.types.ts).

Member

layershifter Aug 24, 2022

FYI: it's currently empty:

It would be great to include currently implemented props from #24023 to the spec.

packages/react-components/react-progress/Spec.md Outdated

    
              ### Slots

              - `root` - The root element of the Progress. The html element is a `div`

              - `indicator` - The div element that gets animated into a Progress. The html element is `span`

Member

layershifter Aug 24, 2022

I know that we call this part indicator in few other places, but few other libraries call this part bar. Curiosity: what is a motivation to call it indicator? 🐱 Taking into account that there is a prop called barThickness in #24023.

packages/react-components/react-progress/Spec.md

Comment on lines 65 to 79

    
              ```html

              <div class="fui-Progress">

                <!-- Label for Progress -->

                <label className="fui-Progress__label">Loading...</label>

                <span className="fui-Progress__indicator" role="progressbar">

                  <!-- div that receives the animation. Classnames are used for animation -->

                  <div className="fui-Progress__Container">

                    <div className="fui-Progress__Track" />

                    <div className="fui-Progress__Tail" />

                  </div>

                </span>

                <!-- Label for Progress description -->

                <label className="fui-Progress__description">Loading Text</label>

              </div>

              ```

Member

layershifter Aug 24, 2022

From markup I see that we will not allow to pass children. Is it correct? If so, I would mention this somewhere in the spec.

packages/react-components/react-progress/Spec.md

    
              ## API

              _List the **Props** and **Slots** proposed for the component. Ideally this would just be a link to the component's `.types.ts` file_

              ### Slots

Member

layershifter Aug 24, 2022

It would be great to include there an image with a component's anatomy so it would be clearer which slots correspond to what area.

khmakoto added Component: Progress Fluent UI react-components (v9) labels

khmakoto reviewed

View reviewed changes

packages/react-components/react-progress/Spec.md Outdated Show resolved Hide resolved

packages/react-components/react-progress/Spec.md Outdated

    
              - **Display** - The Progress will use the following priority:

              ## Accessibility

                - Adding `appearance={inverted}` will alter the way that the Progress is displayed.

Member

khmakoto Sep 14, 2022

This says that appearance={inverted} will alter the way Progress is displayed but doesn't say how. Can we add that?

packages/react-components/react-progress/Spec.md Outdated Show resolved Hide resolved

packages/react-components/react-progress/Spec.md Outdated Show resolved Hide resolved

behowell assigned tomi-msft and unassigned layershifter


          Apply suggestions from code review

c9061e9

Co-authored-by: Makoto Morimoto <[email protected]>
Co-authored-by: Oleksandr Fediashov <[email protected]>

github-actions bot modified the milestones: July Project Cycle Q3 2022, September Project Cycle Q1 2022

tomi-msft added 2 commits

September 27, 2022 15:48


          Update spec

4ed0b00


          Update SPEC

93e0725

tomi-msft mentioned this pull request

Progress #24980

Closed

21 tasks

khmakoto approved these changes

View reviewed changes

packages/react-components/react-progress/Spec.md

    
              ## Prior Art

              _Include background research done for this component_

              ### Open UI

Member

khmakoto Sep 29, 2022

@tomi-msft you can add a link to Open UI if there is a page about Progress there. If a page for progress does not exist feel free to rename this section to something like "Comparison between other libraries".

behowell reviewed

View reviewed changes

packages/react-components/react-progress/Spec.md

    
              ### Props

              See API at [Progress.types.tsx](./src/components/Progress/Progress.types.ts).

Contributor

behowell Sep 30, 2022

I wonder if we should make the props align with the HTML <progress> element:

Use value instead of percentComplete
Add a max prop that defaults to 1.0
Remove the indeterminate prop, and just use the value prop: the progress bar is indeterminate if value is undefined.

behowell reviewed

View reviewed changes

packages/react-components/react-progress/Spec.md

    
              ## API

              _List the **Props** and **Slots** proposed for the component. Ideally this would just be a link to the component's `.types.ts` file_

              ### Slots

Contributor

behowell Sep 30, 2022

We might want to use Field to add support for the label and description. It also looks like the visual spec includes support for error and success states, which the Field component already supports.

That would mean:

The Progress control only has two slots:
- root (which would be the track)
- bar (child of root)
There's a new ProgresField control that wraps Progress and adds the features of Field: label, helperText, validationState, etc.

That would follow the pattern of our other form controls, which (mostly) don't include labels built-in. And it would also benefit from sharing all of the features of Field.

Member

khmakoto Sep 30, 2022

I agree with this but I think we discussed maybe making those changes to both spec and implementation in a separate PR.


          Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

9a2153f

…c-react into add-progress-spec

github-actions bot modified the milestones: September Project Cycle Q1 2022, October Project Cycle Q4 2022

Contributor Author

tomi-msft commented Oct 3, 2022

The initial SPEC has been reviewed and approved, so I will merge this PR, and take the comments left by @behowell and make a separate PR with those changes

tomi-msft mentioned this pull request

refactor Progress to remove label and description slots #25067

Merged

tomi-msft merged commit 8fd9720 into microsoft:master

marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request


          Merge branch 'master' into marcosmoura/feat/selectable-cards

3e5d385

* master: (62 commits)
  chore(migrate-converged): add functionality to execute various v9 package file restructuring tasks (microsoft#24458)
  chore(react-dialog): updates stories (microsoft#25070)
  refactor Progress to remove label and description slots (microsoft#25067)
  fix(SplitButton): Remove borders from hover and pressed state in primary split buttons (microsoft#25059)
  chore(react-persona): Add vr, conformance, and unit tests (microsoft#25007)
  (docs): update Icon docs with examples (microsoft#24768)
  Split button/primary hc fix (microsoft#25066)
  chore(react-utilities): restricts trigger API types (microsoft#25044)
  Add utilities export @fluentui/style-utilities v8 (microsoft#25065)
  chore: migrate whole repo to webpack 5 (microsoft#24542)
  applying package updates
  BREAKING: refactor `useTable` to be composable (microsoft#24947)
  Added shims to shim packages (microsoft#25048)
  Add Progress SPEC (microsoft#24418)
  applying package updates
  chore(react-persona): Add bundle-size command for bundle-size fixtures (microsoft#25055)
  fix(Button): Remove margin added to buttons by Safari (microsoft#25052)
  fix: Menu triggers no longer take focus when they are mounted (microsoft#25016)
  chore: deletes react-trigger package (microsoft#25042)
  Fixed a minor typo: immmediately => immediately (microsoft#25036)
  ...

NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request


          Add Progress SPEC (microsoft#24418)

6d8bddf

* Add Progress SPEC

* Apply suggestions from code review

Co-authored-by: Makoto Morimoto <[email protected]>
Co-authored-by: Oleksandr Fediashov <[email protected]>

* Update spec

* Update SPEC

Co-authored-by: Makoto Morimoto <[email protected]>
Co-authored-by: Oleksandr Fediashov <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Progress Fluent UI react-components (v9)