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.

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

feat(progress-bar): add alignment variants #10769

Merged
merged 5 commits into from
Mar 15, 2022
Merged

feat(progress-bar): add alignment variants #10769

merged 5 commits into from
Mar 15, 2022

Conversation

janhassel
Copy link
Member

Ref #10670

Adds a new prop (type) to the ProgressBar component and adds styling necessary to render the different alignment variants.

Changelog

New

  • Add props.type to ProgressBar
    • Options: default, inline and indented
  • Add respective styles

Testing / Reviewing

Verify visuals with storybook.

Just want to make sure that type is the preferred prop name right now / for v11. The discussion in #9623 doesn't seem to have been fully concluded so I compared the number of components using type vs. kind with https://carbon-react-explorer.vercel.app/props and decided on type based on that.
Let me know if there has been a decision I on this topic I'm not aware of.

@thyhmdo Can you please confirm that the "inline" alignment variant should never have a visible helper text? I couldn't find any explicit guidance around this but also didn't see any mockups with it applied so I assumed a helper text is not supported in that case.

@janhassel janhassel requested review from a team as code owners February 16, 2022 15:00
@netlify
Copy link

netlify bot commented Feb 16, 2022 •
edited
Loading

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 5e3c6eb

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6230c2a337e5630009d13b7b

😎 Browse the preview: https://deploy-preview-10769--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Feb 16, 2022 •
edited
Loading

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 5e3c6eb

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

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

@netlify
Copy link

netlify bot commented Feb 16, 2022 •
edited
Loading

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

🔨 Explore the source changes: 5e3c6eb

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

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

@abbeyhrt abbeyhrt requested a review from thyhmdo March 1, 2022 21:10
@abbeyhrt
Copy link
Contributor

abbeyhrt commented Mar 1, 2022

@janhassel thank you for doing this! I would need a designer to confirm but comparing indented to the alignment varients pictured here

it seems like the label is indented a bit too much in the storybook example.

Screen Shot 2022-03-01 at 3 13 18 PM.

It almost looks like the padding-left for the label is 8px and padding left for the optional text is 16, but there is no example of the two used together for the indented variant so i'm not sure .

@thyhmdo
Copy link
Member

thyhmdo commented Mar 2, 2022

@janhassel Sorry coming to this late.

  • Not sure where we landed with the variants either. Since we called the indeterminate and determinate progress bars as variants, I'm not sure if variants or variations would make sense here. But I think we can always fix that later.
  • Yes, the inline variant won't have the optional helper text.
  • Here are the specs and I will update this on the Style tab. The thing is it looks like either the Optional helper text looks quite close to the progress bar or the label is far away from the progress bar. It should apply 8px for spacing between the progress bar and the text in both Big and Small progress bar cases.

@abbeyhrt I don't see the indented version on react. How can I view that?

image

progress-bar-style-4

@janhassel
Copy link
Member Author

@thyhmdo

You can check out the different alignment variants by going to the "Playground" story and changing the "Type" option in the knobs panel. Here's a direct link to the indented version: https://deploy-preview-10769--carbon-components-react.netlify.app/?path=/story/experimental-unstable-progressbar--playground&knob-Type%20(type)=indented

You're right, the helper text is currently too close to the progress bar. I noticed that too and corrected it together with the work in #10843. So once the statuses are finalized, this bug will also be fixed 🙂

@thyhmdo
Copy link
Member

thyhmdo commented Mar 2, 2022

thanks Jan!

@janhassel
Copy link
Member Author

Let me know if you have any questions around this PR @abbeyhrt @aledavila @thyhmdo

@thyhmdo
Copy link
Member

thyhmdo commented Mar 9, 2022 •
edited
Loading

thanks @janhassel for updating. Just one thing. The #10843 looks good to me. The indent alignment does not appear on that doc but on this issue instead. I may have to wait for something to be deployed. Other than that, it looks good!

abbeyhrt
abbeyhrt approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

Looks good to me code-wise! Thank you for the contribution!

aledavila
aledavila approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@aledavila aledavila left a comment

Choose a reason for hiding this comment

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

indentations look good to me.

@thyhmdo you should be able to find them visually here. and change them under knobs and then under type. https://deploy-preview-10769--carbon-components-react.netlify.app/?path=/story/experimental-unstable-progressbar--playground

Let me know if you can't see them and I can help out more :)

@janhassel
Copy link
Member Author

@thyhmdo Basically the two pull requests handle two different topics: The alignment variants in this one and the statuses in the other one. As long as both are open and have not yet been merged their features will not be available in the respective other one.

So as soon as this pull request is merged I can update the second one and then the alignment variants will be available as a preview in combination with the statuses.

Let me know if I misunderstood your question!

thyhmdo
thyhmdo approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@thyhmdo thyhmdo left a comment

Choose a reason for hiding this comment

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

looks good to me!

@kodiakhq kodiakhq bot merged commit e3f7edc into carbon-design-system:main Mar 15, 2022
@janhassel janhassel deleted the 10670-alignment-variants branch March 15, 2022 18:03
kennylam pushed a commit to kennylam/carbon that referenced this pull request Jul 30, 2024
@ariellalgilmore
fix(styling): missing tokens (carbon-design-system#10769)
35c5ca6 
* fix(styling): missing tokens

* fix(search): hover

* fix(search): format and remove pointer events

* fix(data-table): hover

* fix(search): expandable size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aledavila aledavila aledavila approved these changes

@abbeyhrt abbeyhrt abbeyhrt approved these changes

@thyhmdo thyhmdo thyhmdo approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@janhassel @abbeyhrt @thyhmdo @aledavila @tay1orjones