-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ui-library): harmonised property names #980
fix(ui-library): harmonised property names #980
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in Storybook, cant evaluate the other parts though
One small thing we could fix, while changing the index.stories.ts for the counter would be the text in the "Variant" section: In the text we are saying there is a "default" variant, but it is actually called "neutral".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
26b2a96
@property() current = 0; | ||
@property() max = 0; | ||
@property() size?: FormSizesType = 'md'; | ||
@property({ type: Number }) value = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be more consistent because the type is defined for some and for some its missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only do that in these case where lit analyze is barking. Apart from that, this is a topic we still need to talk about anyway. From consistent I agree with you.
const dynamicStyles = this.theme === 'Light' ? [counterLight] : [counterDark]; | ||
|
||
const classes = classMap({ | ||
'blr-counter': true, | ||
[this.variant]: this.variant, | ||
[`${this.size}`]: this.size, | ||
[`${this.sizeVariant}`]: this.sizeVariant, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, does this need to be in template literal, can't it be just like in line number 24?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* fix(ui-library): harmonized property names * fix(ui-library): added changes after review * fix(storybook): changed values after review * fix(ui-library): removed lit Types --------- Co-authored-by: Christian Hoffmann <[email protected]> Co-authored-by: Thorben Hartmann <[email protected]>
* fix(ui-library): harmonized property names * fix(ui-library): added changes after review * fix(storybook): changed values after review * fix(ui-library): removed lit Types --------- Co-authored-by: Christian Hoffmann <[email protected]> Co-authored-by: Thorben Hartmann <[email protected]>
#876