adding custom width support to meter and progressbar #1560

ktabors · 2021-02-09T06:31:19Z

Changed the width to min-width and added inline size.

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Goto the meter and/or progress bar stories
Check out the new width stories
Also check the other stories like side labels without width because that was a problem case when making the change.

🧢 Your Project:

RSP

adobe-bot · 2021-02-09T20:32:10Z

Build successful! 🎉

adobe-bot · 2021-02-09T22:56:45Z

Build successful! 🎉

snowystinger

Looks good in chrome/safari/ff on mac

LFDanLu · 2021-02-10T00:39:37Z

packages/@adobe/spectrum-css-temp/components/barloader/index.css

@@ -18,15 +18,17 @@ governing permissions and limitations under the License.
  flex-flow: row wrap;
  justify-content: space-between;
  align-items: center;
-  width: var(--spectrum-barloader-large-width);
+  min-width: var(--spectrum-barloader-large-width);


Just wanna double check: did we check w/ spectrum regarding this min-width here? I noticed that spectrum css has a min-width of 192px applied to their progress bars but only if it has a side label not if the label is on top. You can try it out by modifying the widths here

IMO we should def have a min-width so that weird label and number value stacking doesn't happen but do we want a smaller value? 192px feels kinda large. This is 100px:

LFDanLu · 2021-02-10T00:51:06Z

packages/@react-spectrum/progress/stories/ProgressBar.stories.tsx

+    'width: 30px, labelPosition: side, isIndeterminate: true',
+    () => render({width: '30px', labelPosition: 'side', isIndeterminate: true})


I think the css for the labelPosition: side will need to account for the case in which the width applied is less than the min-width. At the moment the wrapping div (.spectrum-BarLoader) will be of width 192px, but that doesn't account for the label width and thus the following can happen when there are sibling elements (aka the button is hidden by the progress bar):

I think spectrum-css styles might suffer from the same issue. Maybe a min-width: fit-content and have the label have a max width so it calculates properly? Open to opinions for this one

adobe-bot · 2021-02-10T03:54:01Z

Build successful! 🎉

adobe-bot · 2021-02-18T00:47:48Z

Build successful! 🎉

LFDanLu

Overall looks fine to me, just one question though

LFDanLu · 2021-02-19T19:50:38Z

packages/@adobe/spectrum-css-temp/components/barloader/index.css

+  &.spectrum-BarLoader--indeterminate {
+    max-width: var(--spectrum-barloader-large-width);
+  }


Where did this come from?

Indeterminate progress bars are locked to 192px until we change the animations to work with custom widths.

gotcha, that should fine then

snowystinger · 2021-02-23T23:42:46Z

packages/@adobe/spectrum-css-temp/components/barloader/index.css

+  min-width: fit-content;
+
+  .spectrum-BarLoader-track {
+    width: auto;
+  }


while one of these seems needed, where is this coming from?
also, line 70 has inline-size: auto, which is width: auto, i'm surprised it's needed for sideLabel and for track? lets be consistent and use inline-size
we can also use inline-size anywhere we have width, so min-width -> min-inline-size

So this line prevents the long label, labelPosition: side story from having a track larger than 192px. I appears the track auto width without the auto width in the side label parent works just fine, so I'll remove that. And I'll update to inline-size. :)

adobe-bot · 2021-02-24T21:45:18Z

Build successful! 🎉

LFDanLu · 2021-03-09T21:39:20Z

packages/@adobe/spectrum-css-temp/components/barloader/index.css

@@ -18,15 +18,21 @@ governing permissions and limitations under the License.
  flex-flow: row wrap;
  justify-content: space-between;
  align-items: center;
-  width: var(--spectrum-barloader-large-width);
+  min-inline-size: var(--spectrum-barloader-large-width);


Do we really want to restrict the meter/progressbar size so that it must be greater than 192px?
I feel like we had discussed this before but I can't find that discussion anymore (and of course I resolved my own comment previously without adding details zzzz).

I think this was at direction from Spectrum when this component was first created, but I can't find that historical knowledge.

min width 48px in both scales. https://jira.corp.adobe.com/browse/SDS-5040

ktabors · 2021-04-15T23:46:28Z

packages/@adobe/spectrum-css-temp/components/barloader/index.css

@@ -18,15 +18,27 @@ governing permissions and limitations under the License.
  flex-flow: row wrap;
  justify-content: space-between;
  align-items: center;
-  width: var(--spectrum-barloader-large-width);
+  min-inline-size: 48px;


DNA hasn't created a token so I'm using 48px. Is there a different token I should use?

size-600 is 48px at medium

used 600 static because they want 48px for medium and large scales.

adobe-bot · 2021-04-15T23:57:21Z

Build successful! 🎉

snowystinger · 2021-04-19T18:46:43Z

packages/@react-spectrum/progress/stories/ProgressBar.stories.tsx

+    () => render({width: '30px', labelPosition: 'side', label: 'Super long progress bar label. Sample label copy. Loading...'})
+  )
+  .add(
+    'width: 30px, labelPosition: side, isIndeterminate: true, long label, button on left',


button is on the right?

adobe-bot · 2021-04-19T18:49:31Z

Build successful! 🎉

snowystinger

lgtm

adobe-bot · 2021-04-19T20:02:19Z

Build successful! 🎉

ktabors · 2021-04-19T20:15:33Z

lgtm

I found a bug

snowystinger · 2021-04-19T20:18:20Z

what's the bug?

ktabors · 2021-04-19T20:22:44Z

what's the bug?

The track is 48px in this story, not 192px. indeterminate should always be 192px.
https://reactspectrum.blob.core.windows.net/reactspectrum/6c80d7aa00642ebdb13e63c61f08d71041bf12ba/storybook/index.html?path=/story/progress-progressbar--width-30px-labelposition-side-isindeterminate-true-long-label-button-on-right

It's now back so indeterminate is always 192px. I just pushed a fix.

adobe-bot · 2021-04-19T20:30:59Z

Build successful! 🎉

snowystinger

gotcha, now it looks right

devongovett · 2021-04-20T02:08:33Z

indeterminate should always be 192px

Why? It seems weird that indeterminate would have a different behavior from determinate. Especially if someone changed between them at runtime.

snowystinger · 2021-04-20T05:37:34Z

@devongovett yep, I'd love for them to be any width too adobe/spectrum-css#497

devongovett

Ok going to merge this. Not sure how common the case of switching between indeterminate and determinate is, but we should follow up on that.

adding custom width support to meter and progressbar

9974bf0

ktabors requested review from dannify, devongovett and snowystinger as code owners February 9, 2021 06:31

adding some progress bar small width stories

a1eeee6

setting control min-width on container

3e0a9a6

snowystinger previously approved these changes Feb 9, 2021

View reviewed changes

LFDanLu requested changes Feb 10, 2021

View reviewed changes

Adding a fit-content min width for side labels

41047c8

ktabors dismissed snowystinger’s stale review via 41047c8 February 10, 2021 03:43

ktabors added 3 commits February 11, 2021 15:39

adding more long label stories and fixing side label track width

096e3fa

making sure all indeterminate tracks have a max-width

e8349fe

fixing a lint error in progressbar storybook

e66ef22

LFDanLu reviewed Feb 19, 2021

View reviewed changes

LFDanLu previously approved these changes Feb 23, 2021

View reviewed changes

snowystinger reviewed Feb 23, 2021

View reviewed changes

updating css to use inline-size instead of width

bb897c6

ktabors dismissed LFDanLu’s stale review via bb897c6 February 24, 2021 21:35

LFDanLu reviewed Mar 9, 2021

View reviewed changes

devongovett added the waiting Waiting on Issue Author label Mar 18, 2021

adding the min-width to progressbar

af39c21

ktabors removed the waiting Waiting on Issue Author label Apr 15, 2021

ktabors commented Apr 15, 2021

View reviewed changes

switching to static 48px variable

cf76598

snowystinger reviewed Apr 19, 2021

View reviewed changes

fixing a story name

6c80d7a

snowystinger previously approved these changes Apr 19, 2021

View reviewed changes

fixing a side label, min-width, indeterminate bug

be35c54

ktabors dismissed snowystinger’s stale review via be35c54 April 19, 2021 20:19

snowystinger approved these changes Apr 19, 2021

View reviewed changes

devongovett approved these changes Apr 20, 2021

View reviewed changes

devongovett merged commit 113b3c9 into main Apr 20, 2021

devongovett deleted the progress_bar_width branch April 20, 2021 23:21

ktabors mentioned this pull request Apr 21, 2021

Indeterminate Progressbar support dynamic widths #1818

Open

majornista pushed a commit to majornista/react-spectrum-v3 that referenced this pull request May 19, 2021

adding custom width support to meter and progressbar (adobe#1560)

db421dd

		'width: 30px, labelPosition: side, isIndeterminate: true',
		() => render({width: '30px', labelPosition: 'side', isIndeterminate: true})

adding custom width support to meter and progressbar #1560

adding custom width support to meter and progressbar #1560

Conversation

ktabors commented Feb 9, 2021

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

adobe-bot commented Feb 9, 2021

adobe-bot commented Feb 9, 2021

snowystinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Feb 10, 2021

adobe-bot commented Feb 18, 2021

LFDanLu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktabors Mar 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Apr 19, 2021

snowystinger left a comment

Choose a reason for hiding this comment

adobe-bot commented Apr 19, 2021

ktabors commented Apr 19, 2021

snowystinger commented Apr 19, 2021

ktabors commented Apr 19, 2021

adobe-bot commented Apr 19, 2021

snowystinger left a comment

Choose a reason for hiding this comment

devongovett commented Apr 20, 2021

snowystinger commented Apr 20, 2021

devongovett left a comment

Choose a reason for hiding this comment

ktabors Mar 29, 2021 •

edited