-
Notifications
You must be signed in to change notification settings - Fork 233
chore(alert-banner): refactor AlertBanner to extend AlertBannerBase #5664
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
Conversation
…nd add variant handling
|
📚 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.
This change introduces a breaking update by removing exports from AlertBanner.ts
and moving them solely to AlertBanner.base.ts
. While I agree that consolidating types into a dedicated type definition file is our right long-term approach, we should ensure backward compatibility in the interim.
To avoid disruption for existing consumers, please add re-exports in AlertBanner.ts
. The types should continue to be defined in the base file, but re-exported from the original entry point until we can clean this up more formally.
@@ -12,7 +12,7 @@ | |||
|
|||
import { html, TemplateResult } from '@spectrum-web-components/base'; | |||
|
|||
import { AlertBannerVariants } from '@spectrum-web-components/alert-banner'; | |||
import { AlertBannerVariants } from '@spectrum-web-components/alert-banner/src/AlertBanner.base.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.
This is not needed
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.
Done
@@ -11,7 +11,7 @@ | |||
*/ | |||
|
|||
import { html, TemplateResult } from '@spectrum-web-components/base'; | |||
import { AlertBannerVariants } from '@spectrum-web-components/alert-banner'; | |||
import { AlertBannerVariants } from '@spectrum-web-components/alert-banner/src/AlertBanner.base.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.
This is not needed
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.
Done
packages/alert-banner/src/index.ts
Outdated
@@ -10,3 +10,4 @@ | |||
* governing permissions and limitations under the License. | |||
*/ | |||
export * from './AlertBanner.js'; | |||
export * from './AlertBanner.base.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.
No need to export base from here.
import '@spectrum-web-components/button/sp-close-button.js'; | ||
import '@spectrum-web-components/icons-workflow/icons/sp-icon-alert.js'; | ||
import '@spectrum-web-components/icons-workflow/icons/sp-icon-info.js'; | ||
import styles from './alert-banner.css.js'; | ||
|
||
const VALID_VARIANTS = ['neutral', 'info', 'negative']; | ||
export type AlertBannerVariants = (typeof VALID_VARIANTS)[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.
Can you add the re-export pattern in AlertBanner.ts? Type should be defined in base but re-exported for backward compatibility so existing consumers don't break.
import type { AlertBannerVariants } from './AlertBanner.base.js';
export type { AlertBannerVariants };
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.
Done!
…5664) * chore(alert-banner): refactor AlertBanner to extend AlertBannerBase and add variant handling * chore(alert-banner): export AlertBannerBase and update imports in stories * chore(alert-banner): minor fix * chore(alert-banner): minor fix * chore(alert-banner): minor fix * chore(alert-banner): update import paths for AlertBannerVariants
…5664) * chore(alert-banner): refactor AlertBanner to extend AlertBannerBase and add variant handling * chore(alert-banner): export AlertBannerBase and update imports in stories * chore(alert-banner): minor fix * chore(alert-banner): minor fix * chore(alert-banner): minor fix * chore(alert-banner): update import paths for AlertBannerVariants
Overview
This PR refactors the
sp-alert-banner
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/alert-banner/src/AlertBannerBase.ts
as the base class forAlertBanner
component.Code Separation
AlertBannerBase (Abstract Base Class)
packages/alert-banner/src/AlertBannerBase.ts
open
,dismissible
,variant
)shouldClose
,close
,handleKeydown
)renderIcon
method for subclasses to implementAlertBanner (Concrete Class)
packages/alert-banner/src/AlertBanner.ts
render()
method)renderIcon()
method)Impact
Customer Impact
Developer Impact
import { AlertBanner } from '@spectrum-web-components/alert-banner'
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