[Progress] New Progress components#470
Conversation
9a8691b to
8426306
Compare
Netlify deploy preview |
18f24f1 to
4a3e1a1
Compare
4a3e1a1 to
695fad0
Compare
9186d7e to
7fef12c
Compare
|
@colmtuite The implementation is complete, what do you think about the demos? |
7fef12c to
f07c004
Compare
| export function Styles() { | ||
| const isDarkMode = useIsDarkMode(); | ||
| return ( | ||
| <style suppressHydrationWarning>{` |
There was a problem hiding this comment.
What this suppression for? If there are differences due to encoding of some characters, try <style dangerouslySetInnerHTML={{ __html: '' }} />
There was a problem hiding this comment.
It was for some encoding issue, but I've fixed the root cause now 😅
| * The direction that progress bars fill in | ||
| * @default 'ltr' | ||
| */ | ||
| direction?: ProgressDirection; |
There was a problem hiding this comment.
The prop is 'direction' ~ e.g. Tabs: https://master--base-ui.netlify.app/base-ui/react-tabs/components-api/#tabs-root-prop-direction
'dir' is the attribute set on the elements via 'direction' 😬
There was a problem hiding this comment.
I must have been hallucinating, then, sorry! :)
| /** | ||
| * @ignore - internal component. | ||
| */ | ||
| export const ProgressContext = React.createContext<ProgressContextValue | undefined>(undefined); |
There was a problem hiding this comment.
It's possible to use the same name for the const and its type:
| export const ProgressContext = React.createContext<ProgressContextValue | undefined>(undefined); | |
| export const ProgressContext = React.createContext<ProgressContext | undefined>(undefined); |
There was a problem hiding this comment.
@michaldudak How can this work? 🤔 I get an error...
All our other components have XXXContextValue as the type name, e.g. export const PopoverContext = React.createContext<PopoverRootContextValue | null>(null)
There was a problem hiding this comment.
OK, let's leave it as it is. I'm trying out a different approach in #468. If we decide to adopt it, I'll change all the occurences.
|
|
||
| let state: ProgressStatus = 'indeterminate'; | ||
| if (Number.isFinite(value)) { | ||
| state = value === max ? 'complete' : 'loading'; |
There was a problem hiding this comment.
I'm not sure about "loading". The progress bar doesn't have to show the loading process. Perhaps 'in-progress' | 'complete'? (plus | 'not-started' for completeness)
There was a problem hiding this comment.
Good idea renaming 'loading' to 'in-progress', it's closer to the component's name and a bit more flexible (e.g. it could represent "uploading")
But for 'not-started', I think we can't really differentiate that with 'in-progress' - 0% could mean a process has started, but no progress has been made yet (or so little that it rounds down to 0%)
There was a problem hiding this comment.
IMO you could make the same argument for complete - it could be 99.9% rounded up to 100%. But I don't insist. Probably best to see with @colmtuite if this third state could be useful for styling.
There was a problem hiding this comment.
We have decided on: 'indeterminate' | 'progressing' | 'complete'
f07c004 to
6ef9e31
Compare
6ef9e31 to
fca034d
Compare
10021b0 to
60c14c1
Compare
60c14c1 to
bda349d
Compare
|
@mj12albert What's the purpose of having 3 parts to this component, rather than just |
@colmtuite Though this isn't a form control, it's still label-able and I thought its more consistent to recommend placing the label inside the root like other components <Progress.Root>
<Label>Label text</Label>
<Progress.Indicator />
</Progress.Root>What do you think? |
Closes #218
Demo: https://deploy-preview-470--base-ui.netlify.app/base-ui/react-progress/