Loader component #241
Loader component #241
Conversation
There was a problem hiding this comment.
See code comments. I found a few discrepancies with the native progress element after reviewing MDN docs: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress
packages/web-components/src/components/cbp-loader/cbp-loader.scss
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/cbp-loader/cbp-loader.tsx
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/cbp-loader/cbp-loader.tsx
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/cbp-loader/cbp-loader.tsx
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/cbp-loader/cbp-loader.tsx
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/cbp-loader/cbp-loader.tsx
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/cbp-loader/cbp-loader.tsx
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/cbp-loader/cbp-loader.scss
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/cbp-loader/cbp-loader.scss
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/cbp-loader/cbp-loader.scss
Outdated
Show resolved
Hide resolved
| @@ -10,14 +10,14 @@ export class CbpLoader { | |||
|
|
|||
| @Element() host: HTMLElement; | |||
|
|
|||
| /** Specifies a unique `ID` for the loader, used to wire up the controls and accessibility features. */ | |||
| @Prop() uid: string = createNamespaceKey('cbp-accordion-item'); | |||
There was a problem hiding this comment.
make this 'cbp-loader' as the prefix and name it "progressId" to be consistent with other IDs. If you just call it uid, it's not clear what it belongs to. (it's on the wrong thing anyway).
| /** Used to set the loader to the 'success' state of the loader */ | ||
| @Prop() success: boolean; | ||
| @Prop({mutable: true}) success: boolean; |
There was a problem hiding this comment.
needs to be reflected too since you're using it as a CSS hook.
|
|
||
| /** Used to set the loader to the 'error' state of the loader */ | ||
| @Prop() error: boolean; | ||
| @Prop({mutable: true}) error: boolean; |
There was a problem hiding this comment.
needs to be reflected too since you're using it as a CSS hook.
| let statusIndicator; | ||
|
|
||
| if (this.color == 'success'){ | ||
| statusDescription = <span>Success</span>; | ||
| if(this.success && !this.error){ |
There was a problem hiding this comment.
you don't need a !this.error in an if else... you're making the error take precedence over success then.
| return ( | ||
| <Host> | ||
| <Host |
There was a problem hiding this comment.
The host is not a semantic element, so the ID doesn't do any good here. The ID should be on the actual progress element. aria-busy can be removed.
| {statusIndicator} | ||
| </span> | ||
| } | ||
| {this.determinate && this.variant == 'circular' && this.size == 'small' && this.color != 'progress' | ||
| {this.determinate && this.variant == 'circular' && this.size == 'small' && (this.success || this.error) | ||
| ? | ||
| statusIndicator | ||
| : | ||
| <progress |
There was a problem hiding this comment.
the progress element needs the ID, which is referenced by the label.
| {this.determinate && this.variant == 'linear' && | ||
|
|
||
| <span class='cbpLoaderDesc'>{statusDescription} | ||
|
|
||
| <label class='cbp-loader-desc'> |
There was a problem hiding this comment.
You might not need the class now that this is a label tag. The label needs to reference the progress' id with `htmlFor={this.progressId}
| null | ||
| ) | ||
| } | ||
| <slot name='cbp-loader-desc' /> |
There was a problem hiding this comment.
This might not need to be a named slot if you don't have a default slot anywhere... I feel like the app could provide the success and error text as well but we can hold that for a later discussion.
Initial creation of the Loader component