Components: Make the ProgressBar public
#61062
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fullofcaffeine
added
[Type] Code Quality
14 tasks
fullofcaffeine
changed the title
ProgressBarProgressBar
|
The only thing I'm concerned about is the hardcoded width. If it's ok with @WordPress/gutenberg-design, I'd like to remove the width constraints so consumers can more easily set their own width using a wrapper div or something. In general, we are designing our components (especially input controls) to have an unconstrained width by default. However, I do understand that a progress bar may have different concerns, so let us know if this would be a problem. |
|
Can there be a default width, and a prop to override it? I think of this similar to the spinner, where it's designed to be a particular size for consistency, but where there's a size prop. |
|
@mirka Are you referring to the |
If I understand correctly, @mirka referees not the progress bar track width, but rather to the entire progress bar component width (which is currently set as 160px through
I'd be fine with a width prop, and then keeping the |
fullofcaffeine
force-pushed
the
remove/wp-components-experimental-designation-for-progressbar
branch
from
13bcad3
to
50e0007
Compare
|
Flaky tests detected in b096ca4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9230871977
|
|
I folks 👋🏻 - I added an optional Screen.Recording.2024-05-21.at.6.14.43.p.m.movI also added a new story to illustrate a progress bar with a custom width, but please let me know if the default story suffices. |
fullofcaffeine
changed the title
ProgressBarProgressBar, add optional custom width prop
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
fullofcaffeine
commented
May 22, 2024
|
Size Change: -89 B (-0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
fullofcaffeine
force-pushed
the
remove/wp-components-experimental-designation-for-progressbar
branch
from
8fc80b0
to
80fc305
Compare
tyxla
reviewed
May 22, 2024
|
Apologies for being late to the party, but I'm not sure how I feel about a I'm also unsure about this 160px default we're setting, because we don't have a ton of in-app usage yet and it might be too early to be prescriptive about a default. It really must depend on the context it's used in, and we don't really have much context yet. I wonder if we can start out by suggesting a default width via documentation? |
Yeah, I can see a few problems with it, the biggest one being that it's currently tied to
Do you mean removing the default Thanks for taking the time to review this! |
fullofcaffeine
force-pushed
the
remove/wp-components-experimental-designation-for-progressbar
branch
3 times, most recently
from
f4713cf
to
6d66847
Compare
@mirka I simplified the implementation as you suggested - it now relies on the immediate container Screen.Recording.2024-05-22.at.5.22.07.p.m.movI updated the docs and added a story depicting changing the parent element width to see how it affects the actual progress bar size. I'm not sure that's the right place for a "parent width" control (considering it's not a prop of the actual component), but I think it works nicely to show/document it and allow the user to see it in practice. Let me know if I got you right and what you think about it. Thanks! |
fullofcaffeine
changed the title
ProgressBar, add optional custom width propProgressBar
tyxla
reviewed
May 27, 2024
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
|
It occurred to me that we should probably only merge this after we decide what to do with the default width/override behavior (#61976). If so, both PRs should be merged at around the same time, in sequence. The reasoning is that if we merge this first and we take longer to decide/merge the other (and a GB release happens in the meantime), there's a (small) risk more consumers will start using it relying on the default width behavior, which might change when we merge subsequent PR. |
…on-for-progressbar
|
In case it helps the conversation, the progressbar was designed to always be a specific width. The fact that it can be shorter or longer than that was not actually a consideration. So a good default would be useful, even if we add flexibility on top of that. |
tyxla
reviewed
May 28, 2024
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
tyxla
approved these changes
May 29, 2024
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
…on-for-progressbar
…on-for-progressbar
|
There's a Playwright test failing, but it's also failing on |
… fix comment opening
fullofcaffeine
deleted the
remove/wp-components-experimental-designation-for-progressbar
branch
|
I forgot to paste the contributor text to the commit description; sorry! The PR had a lengthy list of discussions/messages and missed the warning at the top. Will take private note to not forget it the next time. |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jun 4, 2024
* 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 * Simplify CHANGELOG entry Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Re-export as default for visual consistency * Improve jsdoc comment: make it fit within ~80chars of char lenght and fix comment opening --------- Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
* 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 * Simplify CHANGELOG entry Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Re-export as default for visual consistency * Improve jsdoc comment: make it fit within ~80chars of char lenght and fix comment opening --------- Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: #59418.
What?
This is part of a larger effort to remove
__experimentalprefix from all "experimental" components, effectively promoting them to regular stable components. See the related issue for more context.Why?
The strategy of prefixing exports with
__experimentalhas become deprecated after the introduction of private APIs.How?
__experimentalprefix;__experimentalexport for backwards compatibility;__experimentalin GB and components to the one without the prefix (including in storybook stories). Also, update the docs to refer to the new unprefixed component;id(get it from the storybook URL) to thePREVIOUSLY_EXPERIMENTAL_COMPONENTSconst array inmanager-head.htmlso that old experimental story paths are redirected to the new one;