feat(breadcrumb-trail): task 1 — package scaffolding and element shells#11593
feat(breadcrumb-trail): task 1 — package scaffolding and element shells#11593
Conversation
Set up the experimental @vaadin/breadcrumb-trail package skeleton with three element shells (BreadcrumbTrail, BreadcrumbItem, BreadcrumbTrailOverlay), gated behind the breadcrumbTrailComponent feature flag. Render bodies and base styles are intentionally empty — filled in by subsequent tasks.
|
| import '../vaadin-breadcrumb-item.js'; | ||
| import '../vaadin-breadcrumb-trail-overlay.js'; | ||
|
|
||
| describe('vaadin-breadcrumb-trail feature flag (disabled)', () => { |
There was a problem hiding this comment.
We don't test this in other existing components so IMO having it here is not necessary.
| * This program is available under Apache License Version 2.0, available at https://vaadin.com/license/ | ||
| */ | ||
| import { ElementMixin } from '@vaadin/component-base/src/element-mixin.js'; | ||
| import { ThemableMixin } from '@vaadin/vaadin-themable-mixin/vaadin-themable-mixin.js'; |
There was a problem hiding this comment.
Should we really use ThemableMixin in new components? I forget the reason. I think we could use light DOM styles for new components in Lumo as well. Or what does this mixin do, actually?
There was a problem hiding this comment.
We need LumoInjectionMixin for Lumo, but using ThemableMixin is technically only needed to make the component work with the (deprecated) Flow shadow DOM injection mechanism via components folder.
There was a problem hiding this comment.
Then it definitely doesn't sound like something we should add to new components.
|
Why Similarly, we have |
| /** | ||
| * A mixin providing component-specific behavior for `<vaadin-breadcrumb-trail-overlay>`. | ||
| * | ||
| * Task 1 ships this as an identity mixin; Task 10 fills it in. |
There was a problem hiding this comment.
IMO we should aim to not list "Task 1" etc in the code, it just creates unnecessary noise in the diff.
| * | ||
| * @polymerMixin | ||
| */ | ||
| export const BreadcrumbTrailOverlayMixin = (superClass) => class extends superClass {}; |
There was a problem hiding this comment.
IMO we don't necessarily need this mixin, e.g. AvatarGroupOverlay doesn't have it. The fact that most of components do have it is mainly a leftover from the pre-V25 Polymer / Lit code sharing.
| * This component is experimental and only registers when the | ||
| * `breadcrumbTrailComponent` feature flag is enabled: |
There was a problem hiding this comment.
I'd remove this from JSDoc of all components, as we generally don't list it there. IMO it is enough to have this note is in README and in docs.
| import '../vaadin-breadcrumb-item.js'; | ||
| import '../vaadin-breadcrumb-trail-overlay.js'; | ||
|
|
||
| // The breadcrumb trail elements are experimental and only register when the |
There was a problem hiding this comment.
This comment could be removed, setting feature flag in tests is self-explanatory.
| @@ -0,0 +1,3 @@ | |||
| import './src/vaadin-breadcrumb-trail-overlay.js'; | |||
|
|
|||
| export * from './src/vaadin-breadcrumb-trail-overlay.js'; | |||
There was a problem hiding this comment.
There should be no root-level entrypoints for internal component, so vaadin-breadcrumb-trail-overlay.js and vaadin-breadcrumb-trail-overlay.d.js need to be removed - keep src versions only.
3 participants
Set up the experimental @vaadin/breadcrumb-trail package skeleton with three element shells (BreadcrumbTrail, BreadcrumbItem, BreadcrumbTrailOverlay), gated behind the breadcrumbTrailComponent feature flag. Render bodies and base styles are intentionally empty — filled in by subsequent tasks.