-
Notifications
You must be signed in to change notification settings - Fork 26
fix(ktoaster): deprecate zindex prop, set z-index on toaster container #3093
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,4 +95,23 @@ describe('ToastManager', () => { | |
| toastManager2.destroy() | ||
| toastManager3.destroy() | ||
| }) | ||
|
|
||
| it('creates toasters container with correct default z-index', () => { | ||
| const toastManager = new ToastManager() | ||
| const container = document.getElementById(toastersContainerId) | ||
| expect(container).to.have.css('z-index', '10000') | ||
|
|
||
| // Cleanup | ||
| toastManager.destroy() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: would it make sense to call |
||
| }) | ||
|
|
||
| it('creates toasters container with correct z-index when provided', () => { | ||
| const zIndex = 9999 | ||
| const toastManager = new ToastManager({ zIndex }) | ||
| const container = document.getElementById(toastersContainerId) | ||
| expect(container).to.have.css('z-index', String(zIndex)) | ||
|
|
||
| // Cleanup | ||
| toastManager.destroy() | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -20,13 +20,14 @@ const defaultZIndex = 10000 | |||||
| * Get or create the shared toaster container element. | ||||||
| * Increments the reference count for tracking active instances. | ||||||
| */ | ||||||
| function getOrCreateSharedContainer(): HTMLElement { | ||||||
| function getOrCreateSharedContainer(zIndex: number): HTMLElement { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: make this an object interface in case we need to add more properties later
Suggested change
Also, should the default value be enforced here? |
||||||
| let container = document.getElementById(toasterContainerId) | ||||||
|
|
||||||
| if (!container) { | ||||||
| container = document.createElement('div') | ||||||
| container.id = toasterContainerId | ||||||
| container.setAttribute(containerInstanceCountAttribute, '0') | ||||||
| container.style.zIndex = String(zIndex) | ||||||
| document.body.appendChild(container) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -73,7 +74,8 @@ export default class ToastManager { | |||||
| } | ||||||
|
|
||||||
| // Get or create the shared container | ||||||
| this.sharedContainer = getOrCreateSharedContainer() | ||||||
| const zIndex = options?.zIndex ? options.zIndex : defaultZIndex | ||||||
| this.sharedContainer = getOrCreateSharedContainer(zIndex) | ||||||
|
|
||||||
| // Create this instance's wrapper div | ||||||
| this.instanceWrapper = document.createElement('div') | ||||||
|
|
@@ -83,7 +85,6 @@ export default class ToastManager { | |||||
| // Create the KToaster VNode | ||||||
| this.toaster = createVNode(KToaster, { | ||||||
| toasterState: this.toasts.value, | ||||||
| zIndex: options?.zIndex ? options.zIndex : defaultZIndex, | ||||||
| onClose: (key: string) => this.close(key), | ||||||
| }) | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,8 +33,7 @@ export interface ToasterProps { | |
| toasterState: Array<Toast & { key: string }> | ||
|
|
||
| /** | ||
| * The z-index of the toaster. | ||
| * @default 10000 | ||
| * @deprecated zIndex provided through ToasterOptions is set on the shared container on initialization. This prop is no longer used. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leave the |
||
| */ | ||
| zIndex?: 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.
issue: why is this changing to 1,000 seconds? (I know this is sandbox only)