Implemented BccProgress component #242
Conversation
|
I see there's quite a bit of linting errors and realized there was not yet anything about it in the readme. I've added a section on linting now: #246 |
Fixed @laurensgroeneveld |
| * A `progress` element supporting current and max values | ||
| */ | ||
| export default { | ||
| title: "Forms/BccProgress", |
There was a problem hiding this comment.
I think the Forms section should be reserved for elements that you'd use in an HTML form, like inputs and buttons. I'll admit this one is a bit on the edge but I suggest placing it in Components for now (we can create more subcategories as the component list grows).
| :class="$attrs['class']" | ||
| :style="$attrs['style'] as StyleValue" |
There was a problem hiding this comment.
Vue will add any class and style elements by default on the outer element, so if that's the intention then this is not needed. In some cases, like the Input component, it seemed to make more sense to add classes to the input element itself, not the wrapping div. But in this case I think it's fine to put the classes on here, and then this code is not needed.
| <script lang="ts"> | ||
| export default { | ||
| inheritAttrs: false, | ||
| }; | ||
| </script> |
There was a problem hiding this comment.
See above comment.
| "h-1": props.size === "sm", | ||
| "h-1.5": props.size === "base", | ||
| "h-2": props.size === "lg", |
There was a problem hiding this comment.
These can be custom classes as well, bcc-progress-sm etc. (just to make it easier for people using the library as a CSS library).
|
Hmm I see the Storybook deploy is failing because there is no deploy token. Unfortunately there doesn't seem to be a way other than changing the workflow trigger to change this, but that has a lot of downsides so I don't think that's a good idea. Since you're a member of bcc-code, it should work if the branch is created on the repository (instead of a fork). I've tested it locally though so for this PR it's fine, I'll bypass the failing build when merging 👍 |
|
I did some other updates now to allow full customization, moved it to components and removed the $attrs inheritation with a story for how to fully customize the component. |
| const barClass = computed(() => { | ||
| if (props.barClass) return props.barClass; | ||
| if ( | ||
| typeof progressClass.value === "object" && | ||
| Object.keys(progressClass.value).includes("bcc-progress") | ||
| ) { | ||
| const clone: any = { ...progressClass.value }; | ||
| delete clone["bcc-progress"]; | ||
| return { | ||
| ...clone, | ||
| "bcc-progress-bar": true, | ||
| }; | ||
| } | ||
| return progressClass.value; | ||
| }); |
There was a problem hiding this comment.
I'm a bit ambivalent towards this. On the one hand it's a nice way of handing control to the consumer of the library. On the other hand, an explicit goal of this library is to have a limited set of options, so developers are forced to work within the constraints of the design system (perhaps this goal should be better articulated somewhere). I personally feel this makes the code a bit less readable as well.
I'd be inclined to suggest to not add these class props. Instead, people can override the classes that are already there (like bcc-progress) in the normal CSS way. The Event app (my.event.bcc.no) does this to add dark mode support for example.
What do you think, do you agree with the above reasoning? Maybe we should have a broader discussion about this?
There was a problem hiding this comment.
Agreed, simplified it now 👍🏼
|
Check update @laurensgroeneveld |
Change summary
Change type
Fixes #22
Fixes #238