`ProgressBar`: Simplify default `width` implementation and make it more easily overridable #61976

fullofcaffeine · 2024-05-24T23:57:47Z

Depends on: #61062.

This is a follow-up to #61062. See this comment for more details on how the work has been split: #61062 (comment). You can also read the discussion there to better understand how we arrived at this solution. You can also check the edit history to see the original description for this PR.

What?

Better align the default width behavior with the original component specs and make it easier to override it.

This PR also adds a new story to illustrate a custom width and updates the docs/jsdoc.

Why?

This progress bar was designed to be of a fixed width, so we should keep it. Existing consumers already expect it, too. However, if there are any scenarios where a custom width size/behavior is required, we should provide the means to do so.

How?

Keep the default width of 160px for the progress bar. It's meant to have a fixed size, see this comment;
Stop using max-width to set it. It's not meant to adapt to the parent size, by default;
Instead, use the :where pseud-element (which has zero specificity) to set a default width. This makes it trivial for consumers to override the width with a custom CSS class, if they wish.

Testing Instructions

All tests should pass;
Check the storybook stories for the ProgressBar.
The component should be rendered the same in existing consumers.

…k container" This reverts commit b1fe7cd.

…verride it with CSS

…ers to override it with CSS" This reverts commit 64c72e2.

…on-for-progressbar

…on-for-progressbar * trunk: Update Changelog for 18.4.1 Remove package build-types directories when cleaning (#61939) Bump plugin version to 18.4.1

github-actions · 2024-05-25T00:49:22Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: fullofcaffeine <fullofcaffeine@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

tyxla

To be fair, I think an extra prop that toggles a particular style rule on or off is a worse API than just relying on a classname or inline style.

I also read the discussion in #61062 but couldn't find where and why we decided to have an hasUnconstrainedWidth prop.

I agree with @jasmussen when he said that we should have a default width for consistency.

I also think we should provide a way to override the width, but maybe a size/width prop isn't the best idea since it will prevent the ProgressBar from being fluid and flexible. We do provide consumers the option to add a classname where they can add max-width: unset; or whatever works best for them. I don't think we need an extra prop on top of that.

So I think we should rely on the classname, document how to change the width better (including with fluid and non-fluid options), and either:

Keep the default max-width for consistency
Remove the max-width and rely on the parent width across the editor.

As I agree with @jasmussen that a default width makes sense for consistency reasons, I'd lean on option 1. I know @mirka preferred option 2, but I believe this will add additional costs for maintaining consistency (having to specify max-width or width in multiple places) so I'm curious to hear other thoughts on it.

tyxla · 2024-05-27T10:46:47Z

packages/components/CHANGELOG.md

@@ -5,6 +5,8 @@
 ### Enhancements

 - Components: Make the `ProgressBar` public ([#61062](https://github.com/WordPress/gutenberg/pull/61062)).
+- Components: Improve `ProgressBar` width control ([#61976](https://github.com/WordPress/gutenberg/pull/61976).


Same as the other PR - this needs to use the existing Enhancements section.

mirka · 2024-05-27T13:36:14Z

I think we should rely on the classname

The concern I have with the "just override with classname" approach with things like width, is that it's non-obvious which properties exactly need to be overridden. Is it width, max-width, or both? Plus, as a package we always emphasize that we do not guarantee these styling implementation details to not change. So it's inherently an unstable approach that we'd have to recommend to consumers. Maybe I'm being too idealistic, but it doesn't look clean to me.

For me, it's either:

Have a default width for consistency, but provide a way to safely change it. (Which a prop like hasUnconstrainedWidth achieves)
Have no width constraints.

Though I understand that there are no clear winners here. Both have downsides.

Also for what it's worth, I just realized that the native <progress> element has a default width of 160px in Chrome and Firefox, so that makes me feel a bit better about us having the same default width as that.

… default It has a default `max-width` of 160px, but allows consumers to explicitely disable it so that it expands to fit the parent's width by default. Consumers can then optionally set another width the way they see fit via a custom `className`.

…abled by default" This reverts commit 7a00da3.

packages/components/src/progress-bar/styles.ts

mirka

I think all our deliberation paid off! We thought through every detail, and I'd say we landed on the best solution 🎉 (Fingers crossed!)

Thank you everyone for the great collaboration.

packages/components/src/progress-bar/stories/index.story.tsx

github-actions · 2024-05-28T23:01:20Z

Flaky tests detected in e75e941.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9277256751
📝 Reported issues:

[Flaky Test] submenu click on the arrow interactions #61884 in /test/e2e/specs/editor/blocks/navigation-frontend-interactivity.spec.js

tyxla

Looking good 👍 🚀

packages/components/src/progress-bar/stories/index.story.tsx

DaniGuardiola · 2024-05-29T12:32:24Z

packages/components/src/progress-bar/styles.ts

@@ -38,6 +36,10 @@ export const Track = styled.div`
 	// Windows high contrast mode.
 	outline: 2px solid transparent;
 	outline-offset: 2px;
+
+	:where( & ) {
+		width: 160px;


Not specific to this PR, but is there any reason we're not using rem almost everywhere? It is good practice for proper zoom support, which is an important accessibility feature. @WordPress/gutenberg-components

I've always assumed that this was a "that ship has sailed" situation, since WordPress/Gutenberg is already built entirely on a non-rem basis 😓

DaniGuardiola · 2024-05-29T12:34:24Z

packages/components/src/progress-bar/stories/index.story.tsx

+WithCustomWidth.args = {
+	className: 'custom-progress-bar',
+};
+WithCustomWidth.decorators = [


Another thing that's not specific to this PR, but I wonder why we're still using a legacy CSF version, even for new stuff. Haven't checked our current version, but CSF 3 has been supported for the last couple of versions and it makes Storybook much easier to work with. @WordPress/gutenberg-components

We probably should! Nobody hadn't really prioritized it enough.

DaniGuardiola

Looks good to me! Pending @tyxla's comments, I think this is ready to merge.

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

fullofcaffeine · 2024-05-29T19:13:38Z

Thanks for the collab and thorough review folks! ❤️

…re easily overridable (WordPress#61976) * Remove "experimental" designation for `ProgressBar` * Add optional width prop to override/set the progress bar track container * Revert "Add optional width prop to override/set the progress bar track container" This reverts commit b1fe7cd. * Keep an unconstrained width by default, while allowing consumers to override it with CSS * Revert "Keep an unconstrained width by default, while allowing consumers to override it with CSS" This reverts commit 64c72e2. * Make the component public * Update consumers to import the public component * Expose docs for the now public ProgressBar component * Update CHANGELOG * Run docs:build after docs/manifest.js change * Remove remaining private usage of ProgressBar * Small formatting fix in the changelog * Add basic docs to the README * Add jsdoc * Formatting fix in README jsx block Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Another formatting fix in README jsx block Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * YAFF (yet another formatting fix) in the README jsx Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Improve wording in descripton of the `value` prop in the README Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * YAFF: Missing semicolon in README jsx Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * YAFF: missing semicolon in jsx block in README Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * YAFF: missing semicolon in jsx block in README Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * YAFF: use tab instead of spaces in jsx code block in README Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Move export out of the HOC export section * Fix CHANGELOG: Move entry to the existing enhacements section * Spacing fix Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Sort import alphabetically * Allow `ProgressBar` to have unconstrained width, which is disabled by default It has a default `max-width` of 160px, but allows consumers to explicitely disable it so that it expands to fit the parent's width by default. Consumers can then optionally set another width the way they see fit via a custom `className`. * Revert "Allow `ProgressBar` to have unconstrained width, which is disabled by default" This reverts commit 7a00da3. * Update docs and add story to illustrate a custom width * Update CHANGELOG * Simplify description in the story Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Improve jsdoc in story, do not encourage more customization than needed * Inherit the second story from the main template --------- Co-authored-by: fullofcaffeine <fullofcaffeine@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>

fullofcaffeine added 16 commits May 21, 2024 12:23

Remove "experimental" designation for ProgressBar

023e5c4

Add optional width prop to override/set the progress bar track container

b1fe7cd

Revert "Add optional width prop to override/set the progress bar trac…

fe9129d

…k container" This reverts commit b1fe7cd.

Keep an unconstrained width by default, while allowing consumers to o…

64c72e2

…verride it with CSS

Revert "Keep an unconstrained width by default, while allowing consum…

c88ece0

…ers to override it with CSS" This reverts commit 64c72e2.

Make the component public

ad0fff9

Update consumers to import the public component

dc299ed

Expose docs for the now public ProgressBar component

4c0b8fd

Update CHANGELOG

0ff6637

Merge branch 'trunk' into remove/wp-components-experimental-designati…

ad2fd28

…on-for-progressbar

Run docs:build after docs/manifest.js change

adf39a1

Merge branch 'trunk' into remove/wp-components-experimental-designati…

ceb2840

…on-for-progressbar * trunk: Update Changelog for 18.4.1 Remove package build-types directories when cleaning (#61939) Bump plugin version to 18.4.1

Remove remaining private usage of ProgressBar

9b3db94

Small formatting fix in the changelog

a219fa8

Add basic docs to the README

1d22f24

Add jsdoc

b096ca4

fullofcaffeine changed the base branch from trunk to remove/wp-components-experimental-designation-for-progressbar May 24, 2024 23:58

fullofcaffeine self-assigned this May 25, 2024

fullofcaffeine force-pushed the improve/progress-bar-width-control branch from 6a2626d to bd8c12e Compare May 25, 2024 00:01

fullofcaffeine requested a review from mirka May 25, 2024 00:03

fullofcaffeine added [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality labels May 25, 2024

fullofcaffeine force-pushed the improve/progress-bar-width-control branch from b23ace3 to 7efb431 Compare May 25, 2024 00:12

fullofcaffeine mentioned this pull request May 25, 2024

Components: Make the ProgressBar public #61062

Merged

1 task

fullofcaffeine force-pushed the improve/progress-bar-width-control branch from 7efb431 to 08346ec Compare May 25, 2024 00:24

fullofcaffeine marked this pull request as ready for review May 25, 2024 00:49

fullofcaffeine requested a review from ajitbohra as a code owner May 25, 2024 00:49

tyxla reviewed May 27, 2024

View reviewed changes

fullofcaffeine added 3 commits May 28, 2024 10:55

Revert "Allow ProgressBar to have unconstrained width, which is dis…

a9c174b

…abled by default" This reverts commit 7a00da3.

Update docs and add story to illustrate a custom width

66e8223

fullofcaffeine force-pushed the improve/progress-bar-width-control branch from 08346ec to 66e8223 Compare May 28, 2024 17:35

fullofcaffeine changed the title ~~Components: Improve ProgressBar width control~~ Components: Make default ProgressBar width more easily overridable May 28, 2024

fullofcaffeine changed the title ~~Components: Make default ProgressBar width more easily overridable~~ ProgressBar: Simplify default width implementation and make it more easily overridable May 28, 2024

Update CHANGELOG

3a95761

fullofcaffeine commented May 28, 2024

View reviewed changes

packages/components/src/progress-bar/styles.ts Show resolved Hide resolved

mirka approved these changes May 28, 2024

View reviewed changes

packages/components/src/progress-bar/stories/index.story.tsx Outdated Show resolved Hide resolved

packages/components/src/progress-bar/stories/index.story.tsx Outdated Show resolved Hide resolved

tyxla approved these changes May 29, 2024

View reviewed changes

packages/components/src/progress-bar/stories/index.story.tsx Outdated Show resolved Hide resolved

DaniGuardiola reviewed May 29, 2024

View reviewed changes

DaniGuardiola approved these changes May 29, 2024

View reviewed changes

Base automatically changed from remove/wp-components-experimental-designation-for-progressbar to trunk May 29, 2024 18:48

fullofcaffeine requested review from ndiego, ryanwelcher, juanmaguitar and fabiankaegy as code owners May 29, 2024 18:48

Merge branch 'trunk' into improve/progress-bar-width-control

a076910

fullofcaffeine force-pushed the improve/progress-bar-width-control branch from e75e941 to a076910 Compare May 29, 2024 18:58

fullofcaffeine and others added 3 commits May 29, 2024 12:59

Simplify description in the story

2454c7a

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

Improve jsdoc in story, do not encourage more customization than needed

981ce6f

Inherit the second story from the main template

67d97df

fullofcaffeine merged commit fec4160 into trunk May 29, 2024
61 of 62 checks passed

fullofcaffeine deleted the improve/progress-bar-width-control branch May 29, 2024 22:16

github-actions bot added this to the Gutenberg 18.5 milestone May 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ProgressBar`: Simplify default `width` implementation and make it more easily overridable #61976

`ProgressBar`: Simplify default `width` implementation and make it more easily overridable #61976

fullofcaffeine commented May 24, 2024 •

edited

Loading

github-actions bot commented May 25, 2024 •

edited

Loading

tyxla left a comment

tyxla May 27, 2024

mirka commented May 27, 2024

mirka left a comment

github-actions bot commented May 28, 2024

tyxla left a comment

DaniGuardiola May 29, 2024

mirka May 29, 2024

DaniGuardiola May 29, 2024

mirka May 29, 2024

DaniGuardiola left a comment

fullofcaffeine commented May 29, 2024 •

edited

Loading

ProgressBar: Simplify default width implementation and make it more easily overridable #61976

ProgressBar: Simplify default width implementation and make it more easily overridable #61976

Conversation

fullofcaffeine commented May 24, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented May 25, 2024 • edited Loading

tyxla left a comment

Choose a reason for hiding this comment

tyxla May 27, 2024

Choose a reason for hiding this comment

mirka commented May 27, 2024

mirka left a comment

Choose a reason for hiding this comment

github-actions bot commented May 28, 2024

tyxla left a comment

Choose a reason for hiding this comment

DaniGuardiola May 29, 2024

Choose a reason for hiding this comment

mirka May 29, 2024

Choose a reason for hiding this comment

DaniGuardiola May 29, 2024

Choose a reason for hiding this comment

mirka May 29, 2024

Choose a reason for hiding this comment

DaniGuardiola left a comment

Choose a reason for hiding this comment

fullofcaffeine commented May 29, 2024 • edited Loading

`ProgressBar`: Simplify default `width` implementation and make it more easily overridable #61976

`ProgressBar`: Simplify default `width` implementation and make it more easily overridable #61976

fullofcaffeine commented May 24, 2024 •

edited

Loading

github-actions bot commented May 25, 2024 •

edited

Loading

fullofcaffeine commented May 29, 2024 •

edited

Loading