feat: add badge and counter badge as new components #26357
Conversation
chrisdholt
force-pushed
the
users/chhol/add-badge-as-new-component
branch
from
1a83d34
to
6c2f5aa
Compare
chrisdholt
force-pushed
the
users/chhol/add-badge-as-new-component
branch
from
6c2f5aa
to
e238735
Compare
📊 Bundle size report🤖 This report was generated against be3d30fcbe222be34b02a554e948d14bb2d730df |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 33ce22b:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: be3d30fcbe222be34b02a554e948d14bb2d730df (build) |
| * The template for the Badge component. | ||
| * @public | ||
| */ | ||
| export function badgeTemplate<T extends Badge>(options: BadgeOptions = {}): ElementViewTemplate<T> { |
There was a problem hiding this comment.
This is the first component which export its template function. How do we decide whether we export it or not?
Only when we have a internal use case for it (like in the counter-badge here) or always?
There was a problem hiding this comment.
I think there's value if someone needs to create their own badge or extend the Fluent badge to get all the goodness and not have to recreate. One of the benefits of that would be more shared code and less repeated. Now, in the case that they need to "fork" the template that would result in duplication. I don't think we need to advertise it necessarily, but in terms of enabling teams to reuse and compose, etc...we've found it to be fairly helpful. Text is technically exporting the used template from the index as well.
I'm open.
With that said, in this specific case, it's definitely exported here to enable shared reuse and code for badge + counter badge. Whether we want to export from the index or not we can discuss.
There was a problem hiding this comment.
I agree with this being exported, if we are not sure, we can keep it private and wait for the demand.
Personally I would prefer every component to export its template 👍
| * @public | ||
| * The badge's size styles | ||
| */ | ||
| export const badgeSizeStyles = css.partial` |
There was a problem hiding this comment.
What is the motivation for having the partials away from the component? I would expect these to be in badge, owned by it and other component(s) importing from there.
There was a problem hiding this comment.
I was primarily thinking of keeping shared things in a shared location which has greater visibility than within the component styles file as that could get bulky. Additionally one thought that I had was that not all partials may be as intuitive with regard to what the actual "primary" component this would be inheriting from. Consider scenarios where we have the same visuals across a functional button/anchor for example or across text field/number field, or select and combobox (currently two implementations because the behavior and functionality is different, we'll need to dig in here I know). In those cases, which one does the styles live in? They're kind of both on the same level with shared appearances but where would I look for the shared styles? In my mind having a dedicated place where we keep composable styles or partials made a lot of sense.
There was a problem hiding this comment.
Ok, let's start with a shared location and see how that goes (and tree-shakes).
From my experience, when an engineer will need to update the shared style, they will be afraid to change it and will rather copy it anyway. But I strongly believe we will do better :-D
There was a problem hiding this comment.
Let's strive to do just that!
packages/web-components/src/counter-badge/counter-badge.template.ts
Outdated
Show resolved
Hide resolved
| */ | ||
| /* eslint-disable-next-line */ | ||
| export interface Badge extends StartEnd {} | ||
| applyMixins(Badge, StartEnd); |
There was a problem hiding this comment.
I would appreciate more context and explanation for these.
There was a problem hiding this comment.
This will go away once the Badge base class is updated w/ Start/End slots. Ultimately start and end have refs in case someone needs to access the node or child nodes of start/end from a given class. So, there is a StartEnd class which enumerates the observables for these slots and here we are combining it with the Badge class to ensure that the final class includes everything from Badge + StartEnd support. The interface specifically exists to ensure typings are all good for "Badge" and I'm disabling that line because something in the lint config is converting it to a type.
chrisdholt
force-pushed
the
users/chhol/add-badge-as-new-component
branch
from
4c201ca
to
cf32618
Compare
| * HTML Attribute: overflow-count | ||
| */ | ||
| @attr({ attribute: 'overflow-count', converter: nullableNumberConverter }) | ||
| public overflowCount: number = 99; |
There was a problem hiding this comment.
This is my only remaining concern for the PR - there are still attributes with default values reflected back to DOM.
This is a case which cannot be solved by pure CSS and would require additional JS boilerplate :-(
There was a problem hiding this comment.
Exactly - if we don't need a default we can pull it but this is really the most cost effective way...
@miroslavstastny we could not set it and just have an internal default when not set...but I really don't like that because it seems like a side effect here. Slightly more JS cost too (but only slight)
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
* add badge as a new component * add counter badge as a new component * fix broken import and add export paths * use badge template for counter badge to render count * update badge svg styles and fix import syntax * fixup imports for stories * update partial with default styles * js extension missing from partial imports * update api report
Previous Behavior
Badge did not exist as a Fluent 2 aligned web component for v3.
New Behavior
Now it does - and so does CounterBadge!
Related Issue(s)
n/a