-
Notifications
You must be signed in to change notification settings - Fork 231
chore(badge): refactor with abstracted base class #5648
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
base: spectrum-two-phase-zero
Are you sure you want to change the base?
Conversation
|
1657c9f
to
7f38617
Compare
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
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 you resolve at the failing tests for badge
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 you resolve at the failing tests for badge
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.
Nice work, including the PR description (which I totally copied for my component btw). Left two suggestion that will fix your tests, if you commit them.
7f38617
to
6fba948
Compare
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.
Lost gold treasure map (LGTM)
packages/badge/package.json
Outdated
"./src/Badge.base.js": { | ||
"default": "./src/Badge.base.js", | ||
"development": "./src/Badge.base.dev.js" |
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.
We don't need to add this now as this would change the existing API contract.
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.
Shouldn't we add export * from './Badge.base.js';
in index.ts file as well so that we could use/import objects exported by BadgeBase in stories and tests.
6fba948
to
eda83ff
Compare
export const BADGE_VARIANTS = [ | ||
'accent', | ||
'neutral', | ||
'informative', | ||
'positive', | ||
'negative', | ||
'notice', | ||
'fuchsia', | ||
'indigo', | ||
'magenta', | ||
'purple', | ||
'seafoam', | ||
'yellow', | ||
'gray', | ||
'red', | ||
'orange', | ||
'chartreuse', | ||
'celery', | ||
'green', | ||
'cyan', | ||
'blue', | ||
] as const; | ||
export type BadgeVariant = (typeof BADGE_VARIANTS)[number]; | ||
export const FIXED_VALUES = [ | ||
'inline-start', | ||
'inline-end', | ||
'block-start', | ||
'block-end', | ||
] as const; | ||
export type FixedValues = (typeof FIXED_VALUES)[number]; |
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.
This update will result in a BREAKING CHANGE for consumers who are importing directly from Badge.ts
. To keep our downstream teams unblocked let’s re-export these constants from the current module. This way, we can maintain backward compatibility in the short term.
In Badge.ts
please add this above any other code which uses these symbols
export { BADGE_VARIANTS, BadgeVariant, FIXED_VALUES, FixedValues } from './Badge.base.js';
b408287
to
def26e8
Compare
packages/badge/src/Badge.base.ts
Outdated
export type FixedValues = (typeof FIXED_VALUES)[number]; | ||
|
||
/** | ||
* @element sp-badge-base class |
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.
You are missing JS docs statements on icon slot and text label. Can we add here?
a8ccbe9
to
a412244
Compare
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.
Good work! Once the CI is happy ready to land this
Overview
This PR refactors the
sp-badge
component to separate its base logic from rendering code, following the technical specifications for the Swan migration preparation. This is a purely internal change with no customer impact.Changes Made
File Structure Changes
packages/badge/src/Badge.base.ts
as the base class forBadge
component.Code Separation
BadgeBase (Abstract Base Class)
packages/badge/src/Badge.base.ts
Badge (Concrete Class)
packages/badge/src/Badge.ts
Impact
Customer Impact
Developer Impact
import { Badge } from '@spectrum-web-components/badge'
still worksRelated issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch
,minor
, ormajor
featuresManual review test cases
Manual Testing
Automated Testing
Device review
Migration Preparation
This refactoring prepares the badge component for future migration to the Swan architecture by:
Related