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

feat: add progress-bar component #8720

Merged
merged 8 commits into from
Jun 9, 2021

Conversation

janhassel
Copy link
Member

Closes #3025

Adds a progress bar component. Opening as draft to already discuss the API and markup.
Current visual design is just a first idea of how a progress bar could look in Carbon, would definitely appreciate design input / specifications.

Changelog

New

  • Add ProgressBar component

Changed

  • Update react export and public api snapshots

Testing / Reviewing

open for discussion around the implementation

@netlify
Copy link

netlify bot commented May 19, 2021

Deploy Preview for carbon-elements ready!

Built with commit 3adcb91

https://deploy-preview-8720--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented May 19, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 607caa8

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/60c0d929d153510008b48cbe

😎 Browse the preview: https://deploy-preview-8720--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented May 19, 2021

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables with commit 3adcb91

https://deploy-preview-8720--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented May 19, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 607caa8

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/60c0d9296e8baa000759775c

😎 Browse the preview: https://deploy-preview-8720--carbon-components-react.netlify.app/iframe

@janhassel
Copy link
Member Author

@tw15egan I noticed the following quirks when using <progress>:

  • As soon as you overwrite the native appearance (apperance: none), VoiceOver with Chrome doesn't announce the actual value anymore but claims it's "indeterminate". VoiceOver with Safari works as expected.
  • The individual selectors differ between WebKit and Firefox and require seemingly duplicate style rules as a combined selector such as .bx--progress-bar::-webkit-progress-bar, .bx--progress-bar::-moz-progress-bar { ... } will be ignored by one of the targets.
  • There is no way to animate the bar's width if that would be something design wants
  • Firefox doesn't seem to support gradient backgrounds on the bar element (though I think it's not really problem for Carbon 😄 )

I think it might make more sense to build up the component with regular <div>s and use aria attributes to add the semantics. This could solve all the issues mentioned, even the screen reader compatibility issue with VoiceOver on Chrome (which surprised me the most).

Here is a quick demo regarding this: https://codepen.io/janhassel/pen/8802396583e1b75c55944392faab9152

Screen Recording 2021-05-20 at 11 58 07

Curious what you think!

@tw15egan
Copy link
Collaborator

Oh, that's unfortunate, I would have hoped the native HTML element would have better voice over support 😞 Since those seem like dealbreakers, it seems like we have no choice but to go with div's and ensure we can hit our a11y targets

@janhassel janhassel marked this pull request as ready for review June 1, 2021 07:01
@janhassel janhassel requested review from a team as code owners June 1, 2021 07:01
@sstrubberg
Copy link
Member

I was reading the Carbon Style guide under the Testing section and I noticed the testing you implemented doesn't quite reflect the docs regarding a11y, e2e, and ssr. What are your plans for those?

Other than that, this is legit. Very thorough docs. I'll definitely be leaning on this example as we build out Disclosure.

@janhassel
Copy link
Member Author

@sstrubberg I pushed an update with some testing, though it doesn't fully follow the linked guidance as I'm running into some issues with it:

  • The mentioned commands yarn test:unit, yarn test:a11y and yarn test:ssr don't seem to exist (at least not on the react package or in the general scope)
  • Some of the suggested jsdom test matchers (.toHaveAttribute(), .toBeInTheDocument()) don't exist in the setup (TypeError: expect(...).toBeInTheDocument is not a function)
  • The suggested ssr test recipe doesn't seem to work for me (see below)
ssr test recipe error
import ReactDOMServer from 'react-dom/server';
import ProgressBar from './ProgressBar';

describe('ProgressBar - SSR', () => {
  it('should import ProgressBar in a node/server environment', () => {
    expect(ProgressBar).not.toThrow();
  });

  it('should not use document/window/etc', () => {
    expect(ReactDOMServer.renderToStaticMarkup(ProgressBar)).not.toThrow();
  });
});
 FAIL  packages/react/src/components/ProgressBar/ProgressBar-test.ssr.js
  ● ProgressBar - SSR › should import ProgressBar in a node/server environment

    expect(received).not.toThrow()

    Error name:    "TypeError"
    Error message: "Cannot read property 'className' of undefined"

          15 |
          16 | function ProgressBar({
        > 17 |   className,
             |   ^
          18 |   hideLabel,
          19 |   label,
          20 |   max = 100,

          at ProgressBar (packages/react/src/components/ProgressBar/ProgressBar.js:17:3)
          at Object.<anonymous> (node_modules/expect/build/toThrowMatchers.js:84:11)
          at Object.toThrow (node_modules/expect/build/index.js:338:21)
          at Object.<anonymous> (packages/react/src/components/ProgressBar/ProgressBar-test.ssr.js:15:29)

      13 | describe('ProgressBar - SSR', () => {
      14 |   it('should import ProgressBar in a node/server environment', () => {
    > 15 |     expect(ProgressBar).not.toThrow();
         |                             ^
      16 |   });
      17 |
      18 |   it('should not use document/window/etc', () => {

      at Object.<anonymous> (packages/react/src/components/ProgressBar/ProgressBar-test.ssr.js:15:29)

  ● ProgressBar - SSR › should not use document/window/etc

    expect(received).not.toThrow()

    Matcher error: received value must be a function

    Received has type:  string
    Received has value: ""

      17 |
      18 |   it('should not use document/window/etc', () => {
    > 19 |     expect(ReactDOMServer.renderToStaticMarkup(ProgressBar)).not.toThrow();
         |                                                                  ^
      20 |   });
      21 | });
      22 |

      at Object.<anonymous> (packages/react/src/components/ProgressBar/ProgressBar-test.ssr.js:19:66)

I tried re-cloning the repo in a different folder, install everything from scratch and see if that fixes any of my issues, but sadly it doesn't. Do you have any ideas what could be the issue here?

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

LGTM 👍 just one minor question

@tay1orjones
Copy link
Member

@janhassel no worries on those extra test commands/setup - that guidance is new and imo we can defer that to after this firms up and becomes stable.

Those special jest-dom matchers need an import I believe,

import '@testing-library/jest-dom';

@sstrubberg
Copy link
Member

@sstrubberg I pushed an update with some testing, though it doesn't fully follow the linked guidance as I'm running into some issues with it:

  • The mentioned commands yarn test:unit, yarn test:a11y and yarn test:ssr don't seem to exist (at least not on the react package or in the general scope)
  • Some of the suggested jsdom test matchers (.toHaveAttribute(), .toBeInTheDocument()) don't exist in the setup (TypeError: expect(...).toBeInTheDocument is not a function)
  • The suggested ssr test recipe doesn't seem to work for me (see below)

ssr test recipe error
I tried re-cloning the repo in a different folder, install everything from scratch and see if that fixes any of my issues, but sadly it doesn't. Do you have any ideas what could be the issue here?

Hey, Taylor and I just got done doing a little review session of the PR. We're good to merge and we can take a look at the ssr-test later down the line. I don't want that to be a blocker for your merging. Excellent work! I particularly enjoyed the animation using keyframes :)

@sstrubberg sstrubberg merged commit 2c5d5fc into carbon-design-system:main Jun 9, 2021
@jnm2377 jnm2377 mentioned this pull request Jun 9, 2021
22 tasks
emyarod pushed a commit to emyarod/carbon that referenced this pull request Jun 10, 2021
* feat: add new progress-bar component

* feat(progress-bar): add support for indeterminate variant

* docs(progress-bar): provide two stories and playground

* refactor(progress-bar): use div markup with aria attributes

* docs(progress-bar): add example

* test(progress-bar): add tests

Co-authored-by: Scott Strubberg <sstrubberg@protonmail.com>
@janhassel janhassel deleted the progress-bar branch June 10, 2021 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Progress Bar component
4 participants