Skip to content

feat: Add KSnackbar component and useKSnackbar composable#1205

Open
Prashant-thakur77 wants to merge 54 commits intolearningequality:developfrom
Prashant-thakur77:ksnackbar
Open

feat: Add KSnackbar component and useKSnackbar composable#1205
Prashant-thakur77 wants to merge 54 commits intolearningequality:developfrom
Prashant-thakur77:ksnackbar

Conversation

@Prashant-thakur77
Copy link
Copy Markdown
Contributor

@Prashant-thakur77 Prashant-thakur77 commented Feb 14, 2026

Description

Part Of learningequality/studio#5445

This PR implements the core KSnackbar component and the useKSnackbar composable. This is Part 1 of the migration strategy, focusing on logic, accessibility, and unit testing. (Visual tests and documentation will follow in a subsequent PR).
I have followed the design descriptions given in https://design-system.learningequality.org/snackbars/

Key Features

  1. KSnackbar Component (lib/KSnackbar/KSnackbar.vue):
  • Accessibility: Implements role="alert", dynamic aria-live ('polite'/'assertive'), and focus trapping when the backdrop is active.
  • Responsiveness: Uses useKResponsiveWindow to handle mobile vs desktop positioning.
  • Interactions: Supports action buttons, auto-hide timers, and manual dismissal.
  1. Composable (lib/composables/useKSnackbar/index.js):
  • Implements a singleton state pattern for global snackbar management.
  • Includes forceReuse logic for instant content updates.
  1. Added comprehensive Unit Tests using @testing-library/vue.

Before/after screenshots

This is the testing that i have done in Playground.
Screencast From 2026-02-14 23-41-29.webm

Changelog

  • Description: Add KSnackbar component and useKSnackbar composable for globally-managed notifications with action buttons, auto-hide, backdrop mode, and full keyboard accessibility
  • Products impact: new API
  • Addresses: [Remove Vuetify from Studio] Snackbar studio#5445
  • Components: KSnackbar, useKSnackbar
  • Breaking: no
  • Impacts a11y: yes
  • Guidance: Import and use KSnackbar with the useKSnackbar composable to display global notifications. Place a single KSnackbar component at the root level of your app and bind it to snackbarState from useKSnackbar. Call createSnackbar() from anywhere to display notifications. The component uses useKLiveRegion for screen reader announcements, supports keyboard navigation, and includes focus management with backdrop mode.

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

Comments

@learning-equality-bot
Copy link
Copy Markdown

👋 Thanks for contributing!

We will assign a reviewer within the next two weeks. In the meantime, please ensure that:

  • You ran pre-commit locally
  • All issue requirements are satisfied
  • The contribution is aligned with our Contributing guidelines. Pay extra attention to Using generative AI. Pull requests that don't follow the guidelines will be closed.

We'll be in touch! 😊

@MisRob
Copy link
Copy Markdown
Member

MisRob commented Feb 23, 2026

Hi @Prashant-thakur77, wonderful work as always & I enjoyed watching the recording. I left a few higher-level notes. We'll proceed as discussed here ~ let me know any time you're ready. Thanks a lot.

@Prashant-thakur77
Copy link
Copy Markdown
Contributor Author

@MisRob Sure, currently working on it and will also work on your review.;)

@Prashant-thakur77
Copy link
Copy Markdown
Contributor Author

@AlexVelezLl @MisRob I have created the visual tests and documentation and pushed it,the talk on slack related to specifications i have currently used the one given by kds (and took care for the colors --used the one mentioned in kds color section). I have also tried implementing the ksnackbar in studio and it is working as expected.I think the review can be started after the specifications are confirmed.do tell if any other changes are needed to be addressed.

Copy link
Copy Markdown
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Prashant-thakur77! Thanks a lot for this wonderful work, it looks really solid! I have found some things we can improve on, but in general, it feels really nice. (Apologies for the delay 😅)

Comment on lines +19 to +35
const { createSnackbar, clearSnackbar, snackbarIsVisible, snackbarOptions } = useKSnackbar();

const showPersistent = () => {
createSnackbar({
text: 'Connection lost. Check your network and try again.',
actionText: 'Dismiss',
autoDismiss: false,
actionCallback: () => {},
});
};

return {
snackbarIsVisible,
snackbarOptions,
clearSnackbar,
showPersistent,
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the examples as brief as possible, we can just skip destructuring/returning the properties that we don't use from the useKSnackbar composable.

Copy link
Copy Markdown
Contributor Author

@Prashant-thakur77 Prashant-thakur77 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and also extended to other examples to skip destructuring.And nice take to keep examples simple.will keep in mind for future work.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a feeling, and @MisRob can confirm, but I feel that the KSnackbar page should show the instructions to set up the snackbars on the app, and useKSnackbar should show the instructions of all available tools we have for displaying snackbars (e.g. forceReuse, backdrop, and all examples we have right now in the KSnackbar page), since at the end, it is the composable is tool we will use to actually show the snackbars, not KSnackbar; KSnackbar just acts as a required step to enable the usage of useKSnackbar.

And in the end, I think people would use the useKSnackbar page more, since this is what we as developers will use most; we won't interact with KSnackbar much after its initial setup.

So, I'd say that a good structure would be:
KSnackbar page:

  • Instructions for initial setup.
  • Link to the useKSnackbar flagging that it is the composable the responsible for every interaction with the snackbars.

useKSnackbar page:

  • Link to KSnackbar saying it is required to have it set up before using this composable.
  • All examples and things we can do with snackbars.

Now, on a completely different note (and a question more for @MisRob): Do we really need to expose all current KSnackbar fields? Do we want to leave the option for people to use snackbars in a different way without useKSnackbar? Because the way it is implemented right now, KSnackbar is a completely independent, abstract component, and useKSnackbar acts just as a global store for passing the information to a global KSnackbar that should be set up on the app, but really, KSnackbar can be used independently of useKSnackbar.

The answer for this will depend on how restrictive/flexible we want to be, but I just want to make sure we are following a path consciously, and not just because that's how it was done before. If we keep with this flexible path, though, it'd be great to have some notes saying that people can use it independently if they have more advanced use cases, perhaps? (I can only imagine use cases where we would need to pass a slot to KSnackbar and display, for example, a KIcon next to the text). If not, we can just wire KSnackbar with useKSnackbar, and call useKSnackbar directly inside KSnackbar; this way, we wouldn't need to expose these fields on KSnackbar, and its setup would only be calling <KSnackbar />.

Copy link
Copy Markdown
Contributor Author

@Prashant-thakur77 Prashant-thakur77 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a lot of sense! I'll wait for @MisRob to weigh in, but I'm happy to reorganize once we have a decision.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks a lot, those are good considerations. I saw then note and will follow-up :)

Copy link
Copy Markdown
Member

@MisRob MisRob Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry it took such a long time ~ I needed to check a bit on the current state & wanted to think through it. Thanks again for bringing this @AlexVelezLl, it's excellent points & taking time to chat about it now will save us from future trouble. Let me show a possible direction in a very simplified snippet & let see what you think (I only checked the implementation very high-level) What about something like this?

// App.vue
// no `:isOpen="snackbarIsVisible"` and other bindings needed here
// shows when `useKSnackbar.createSnackbar` called from anywhere in the app

<KSnackbar />

// Component1.vue
// contacts global `KSnackbar` installed in App.vue (default & most common behavior)
// other `useKSnackbar` methods & attributes also by default affect the global instance

setup() {
  const { createSnackbar } = useKSnackbar();
  createSnackbar({ text: ... });
}

// Component2.vue
// doesn't want to use the global snackbar (utilize `:isOpen="snackbarIsVisible"` and other bindings if needed)

<KSnackbar :isOpen="snackbarIsVisible" ... >
  <template #text="{ text }"><KIcon/ >{{ text }}</template>
</KSnackbar />


setup() {
  const { createSnackbar, snackbarIsVisible } = useKSnackbar(global=false);
  createSnackbar({ text: ... });
}

As far as I'm aware, most commonly we will just use the global snackbar so that should be the default behavior and it'd be good if it'd be simple & straightforward to setup. At the same time, it's always a good idea to design API in a way that will scale well. And it seems that this PR is already fairly well setup for this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And @AlexVelezLl, your suggestion around documentation makes sense to me :)

@Prashant-thakur77 I think you can just make sure that basic documentation structure is setup as per Alex's guidance, but (unless you're specifically interested in technical writing :) no need to spend much time on details. I usually do last round on our docs myself so that it's consistent overall & it's easier for me to tweak directly rather than to post many comments. So just working towards some good initial shape for now will do!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also combine the layered composables and the global flag and leave the useKLocalSnackbar private, and we could have:

function _useLocalKSnackbar() {
  // local state defined
  return {
    // ... state, actions, etc.
  };
}

const globalSnackbar = _useLocalKSnackbar();
export function useKSnackbar(global=true) {
  if (global) {
    return globalSnackbar;
  }
  return _useLocalKSnackbar();
}

Copy link
Copy Markdown
Member

@MisRob MisRob Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main benefit of the two KDS composables you suggest Alex would be that we don't need to set the global one in each app. So perhaps that'd be the best argument for them ;) In addition to what you mentioned. From that point of view, yes sound like a nice option to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexVelezLl, I posted my comment ^ before I saw yours :) We were both probably writing at the same time & the page didn't refresh.

We can also combine the layered composables and the global flag..

Unless you see some strong benefit, I think I would prefer to not go with the global flag I originally suggested, compared to the both later options I consider it the weakest one for a number of reasons.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you see some strong benefit, I think I would prefer to not go with the global flag I originally suggested, compared to the both later options I consider it the weakest one for a number of reasons.

No strong preferences, hehe! 😅 Just wanted to describe another alternative.

Alright, so @Prashant-thakur77, apologies for the delay. We will go in the direction of exposing both useKSnackbar and useKLocalSnackbar from KDS. I'd say both composables can live under the same useKSnackbar file, to make the maintenance more straightforward, and then, this module will expose useKSnackbar as default, to be consistent with other composables exposed from KDS, and we can expose useKLocalSnackbar as a named export. This is mostly because the 99.99% of the time, we will be using just useKSnackbar, and useKLocalSnackbar would be rather a special case.

For documentation, let's just include everything on useKSnackbar as we agreed previously here, but let's add a section pointing to the existence of useKLocalSnackbar in case a more granular control over KSnackbar is needed (e.g., using a for the snackbar text). Overall, the external API and installation process will still be the same; the only change for consumers is that they can now use useKLocalSnackbar.

If we are missing something, please let us know!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan :) Go for it & thanks everyone.

val => {
if (props.isOpen && val) {
sendPoliteMessage(val);
setupAutoHide();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set up another timer if only the text changes?

Copy link
Copy Markdown
Contributor Author

@Prashant-thakur77 Prashant-thakur77 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think resetting the timer makes the most sense for the UX. If the text updates while the snackbar is already open, we want to ensure the user has the full display duration to read the new message.But would need more clarificationif this is the right choice?
But also thinking in another direction ,If the snackbar is meant to be a fleeting notification that lasts exactly X seconds regardless of how many times the text updates within that window, then not resetting might be better.What are your thoughts on it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that if we set a text when it's only 0.1s left, then it'd make no sense if we even care about updating the snackbar. This is a super specific use case, though; it seems we only use it for the lost connection backdrop. And in that case, I think it may be worth resetting the timer.

.k-snackbar-action {
display: flex;
align-items: center;
padding-left: 48px;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this padding-left, could we use gap on the flex container? Also, was this 48px agreed with designers?

Copy link
Copy Markdown
Contributor Author

@Prashant-thakur77 Prashant-thakur77 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be confirmed with the designers, I kept the 48px from the original spec(48px was inherited from the original UiSnackbar component), but if there's a different standard spacing I should use, just let me know.And I used gap instead of padding left as mentioned.

Comment on lines +382 to +385
margin-top: -9px;
margin-right: -12px;
margin-bottom: -9px;
margin-left: auto;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we play a bit more by reducing the container's paddings, and adding some padding to the text, instead of adding some negative margins here? If we ever want to add another type of button, this will not fit well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that makes sense to me,Updated with padding to the text and removing the negative paddings.:)

@Prashant-thakur77
Copy link
Copy Markdown
Contributor Author

Hi @Prashant-thakur77! Thanks a lot for this wonderful work, it looks really solid! I have found some things we can improve on, but in general, it feels really nice. (Apologies for the delay 😅)

NO worries for the delay @AlexVelezLl ,well coincidentally this was good for me as i got time for finalizing my gsoc proposal ,now as it's done (refining it for the best😅) Now i think i can work properly on this review and finalize this interesting component.Also regarding the Main Navigation refractor I have worked on it the (notification tab added now) but still some updates are still on the list to be completed. I also do have some Kds questions regarding how a well designed api should look like (like here in this component) and what are some do/s and dont while buildign a kds component and its api.It would be really helpful if you can answer some of those.

@learning-equality-bot
Copy link
Copy Markdown

📢✨ Before we assign a reviewer, we'll turn on @rtibblesbot to pre-review. Its comments are generated by an LLM, and should be evaluated accordingly.

Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid new component with good composable design, accessibility focus, and thorough test coverage.

CI: the only failing check (update-spreadsheet) appears unrelated to this PR. Visual and unit tests structure looks good.

Phase 3 skipped — the PR video shows the snackbar in the playground, but no dev server is available for interactive verification. The display examples (BasicDisplay.vue, WithActionDisplay.vue, LongTextDisplay.vue) provide static visual test coverage which is appropriate for this stage.

Findings:

  • 3 blocking (missing immediate on watcher, assertive announcement not used for backdrop, duration default mismatch in docs)
  • 2 suggestions
  • 1 praise

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

  • Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
  • Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
  • Architecture: duplicated concerns, minimal interfaces, composition over inheritance
  • Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
  • Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
  • Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
  • Checked CI status and linked issue acceptance criteria
  • For UI changes: inspected screenshots for layout, visual completeness, and consistency

restoreFocus();
};

watch([() => props.isOpen, () => props.text], ([isOpen, text], [wasOpen]) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: The watcher on [props.isOpen, props.text] does not use { immediate: true }, so if the component is rendered with isOpen: true from the start, onOpen() never executes. This means: no sendPoliteMessage screen reader announcement, no auto-hide timer, and no focus management.

The display examples (BasicDisplay.vue etc.) render with :isOpen="true" and duration: 0, so the missing timer isn't visible there, but real usage where a component starts open would miss the a11y announcement and auto-dismiss.

The test at line 160 ("resets timer when text changes") also starts with isOpen: true and passes, but only because it's testing that close fires 3s after the text change — the initial timer was never created, so the "reset" label is misleading.

Fix: add { immediate: true } to the watcher, and guard the old-value check for the first invocation (where wasOpen will be undefined):

watch([() => props.isOpen, () => props.text], ([isOpen, text], [wasOpen] = []) => {
  // ... existing logic
}, { immediate: true });

],

setup(props, { emit }) {
const { sendPoliteMessage } = useKLiveRegion();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: The composable only destructures sendPoliteMessage, but the documentation for the backdrop option (in useksnackbar.vue line 240) states: "Also makes the snackbar announce assertively instead of politely for screen readers." This behavior is never implemented — sendAssertiveMessage from useKLiveRegion is never called.

Either implement the assertive announcement when props.backdrop is true (which aligns with the documented behavior and the semantic intent of backdrop mode for critical messages), or update the documentation to remove the claim.

Suggested approach:

const { sendPoliteMessage, sendAssertiveMessage } = useKLiveRegion();
// ...
const announce = (message) => {
  if (props.backdrop) {
    sendAssertiveMessage(message);
  } else {
    sendPoliteMessage(message);
  }
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

name: 'duration',
required: false,
default: '4000',
type: { name: 'number' },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: The documented default for duration here is 4000, but the composable's _getSnackbarOptions (line 8) and the component prop (line 272 of KSnackbar.vue) both default to 5000. Update this to 5000 to match the implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to 5000

.k-snackbar-action {
display: flex;
align-items: center;
margin-left: auto;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: margin-left: auto here is redundant since .k-snackbar-message already has flex-grow: 1, which pushes the action to the end. It could also behave unexpectedly in RTL since there's no corresponding [dir='rtl'] rule to flip it to margin-right: auto. Consider removing it — the flex-grow on the message div already handles the spacing correctly in both directions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

snackbarOptions.value.hideCallback();
}

snackbarIsVisible.value = false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: clearSnackbar sets snackbarIsVisible to false but doesn't reset snackbarOptions. This means callbacks like hideCallback, actionCallback, and onBlur remain referenced until the next createSnackbar call. Consider resetting options after hiding:

function clearSnackbar() {
  if (!snackbarIsVisible.value) return;
  if (typeof snackbarOptions.value.hideCallback === 'function') {
    snackbarOptions.value.hideCallback();
  }
  snackbarIsVisible.value = false;
  snackbarOptions.value = _getSnackbarOptions();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When opening the snackbar, we reset this snackbarOptions, but yes, in general, it'd be safer if we reset this here too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -0,0 +1,78 @@
import { ref, readonly, nextTick } from 'vue';

function _getSnackbarOptions(options = {}) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Good extraction of _getSnackbarOptions as a helper — it centralizes the default shape and avoids repeating the same keys in multiple places. The readonly wrapper on exposed refs is also a nice touch for preventing accidental mutation from consumers.

@AlexVelezLl
Copy link
Copy Markdown
Member

Looks like rtibblesbot has found good points, I have reviewed them, and I think all of them are things we can improve 👐.

@Prashant-thakur77
Copy link
Copy Markdown
Contributor Author

Prashant-thakur77 commented Mar 31, 2026

Thanks for the review, will work on this today. :)

@Prashant-thakur77
Copy link
Copy Markdown
Contributor Author

Prashant-thakur77 commented Apr 1, 2026

Hello @AlexVelezLl ,just found out there is a CI failure on KDateCalendar monthString() uses new Date() + setMonth(10) where JavaScript's setMonth() doesn't clamp the day, so on March 31, setting month to November (which is 30 days) overflows to December 1, returning "December" instead of "November".This was giving the ci failure.As it was not part of the pr ,so Just flagging it here for info.

@AlexVelezLl AlexVelezLl requested a review from rtibblesbot April 1, 2026 19:40
@AlexVelezLl
Copy link
Copy Markdown
Member

Thanks @Prashant-thakur77, yes, that's a flaky test, no need to worry about it.

@learning-equality-bot
Copy link
Copy Markdown

📢✨ Before we assign a reviewer, we'll turn on @rtibblesbot to pre-review. Its comments are generated by an LLM, and should be evaluated accordingly.

@Prashant-thakur77
Copy link
Copy Markdown
Contributor Author

Prashant-thakur77 commented Apr 1, 2026

Hello @AlexVelezLl ,also wanted to ask about the main navigation refractor in which i was working ,i think now the necessary kds releases are done,so ahould i start the main work there?

Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid work implementing KSnackbar and useKSnackbar — the singleton composable pattern, KFocusTrap integration, and KLiveRegion usage are well done. A few issues to address before merge.

CI: lint/tests passing, visual tests pending.

No UI screenshots could be verified (video-only in PR body). Phase 3 skipped for visual comparison — see inline note.

Blocking:

  • _getSnackbarOptions uses || for bottomOffset, which drops 0 as a valid value
  • clearSnackbar is callable when snackbar is not visible but setSnackbarText has no guard — can update invisible snackbar state
  • action-click event naming inconsistent with reviewer feedback requesting camelCase (actionClick)

Suggestions:

  • The watcher handles open/close/text-change in one block — consider splitting or documenting the three branches
  • onBlur as a prop-callback pattern vs. an emitted event
  • Missing test coverage for Esc key dismiss and backdrop focus trapping
  • No screenshots in PR body (only video) — screenshots help reviewers verify static states

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

  • Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
  • Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
  • Architecture: duplicated concerns, minimal interfaces, composition over inheritance
  • Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
  • Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
  • Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
  • Checked CI status and linked issue acceptance criteria
  • For UI changes: inspected screenshots for layout, visual completeness, and consistency

actionCallback: options.actionCallback || null,
duration: options.duration !== undefined ? options.duration : 5000,
autoDismiss: options.autoDismiss !== undefined ? options.autoDismiss : true,
bottomOffset: options.bottomOffset || 0,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: bottomOffset: options.bottomOffset || 0 will coerce a valid 0 value to 0 (which happens to be correct here), but the pattern is fragile — if the default were ever non-zero, || default would silently swallow an explicit 0. More importantly, line 5 has the same || pattern for text: options.text || '' means passing text: 0 or text: null silently becomes '', which is fine, but options.bottomOffset || 0 is the real concern since 0 is a meaningful distinct value for a numeric prop.

Use the same !== undefined guard you already use for duration and autoDismiss:

bottomOffset: options.bottomOffset !== undefined ? options.bottomOffset : 0,

This keeps the pattern consistent within the function.

}
}

function setSnackbarText(text) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: setSnackbarText updates snackbarOptions even when the snackbar is not visible (snackbarIsVisible is false). Unlike clearSnackbar which guards with if (!snackbarIsVisible.value) return, this function silently mutates hidden state. Consider adding a visibility guard, or documenting that it's intentional (e.g., for pre-loading text before showing).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! I think we wouldn't want to update this here if the snackbar is not visible, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes will update it


emits: [
/** Emitted when the action button is clicked. */
'action-click',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The previous reviewer (AlexVelezLl) suggested using camelCase event names (actionClick) for newer components. The existing codebase uses kebab-case (action-click) in older components. Either convention is fine, but it's worth aligning with the team on which to follow here — this is a new component so it's a good opportunity to establish the going-forward convention.

:text="actionText"
:appearanceOverrides="actionButtonStyles"
@click.stop="onActionClick"
@blur.native="$emit('blur', $event)"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The onBlur option is passed via snackbarOptions as a callback prop, then handled by the parent doc page reading snackbarOptions.value.onBlur(). This works, but the pattern is unusual in Vue — the blur event is already emitted here via $emit('blur'), and the parent already has @blur="handleBlur". Consider whether onBlur needs to exist as a composable option at all, since the parent already wires @blur to the handler. The callback-in-options pattern adds a second path for the same behavior.

restoreFocus();
};

watch(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Good use of a consolidated watcher with three clear branches (open, close, text-change-while-open). This avoids the double-announcement bug that the previous review flagged with separate watchers, and the { immediate: true } ensures the initial state is handled.

></div>
</transition>

<KFocusTrap
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Good use of KFocusTrap with the :disabled prop — this is cleaner than the manual focus trapping from the earlier iteration and follows the reviewer's suggestion.

return;
}

if (snackbarIsVisible.value) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: When replacing a visible snackbar (lines 53-60), the code sets snackbarIsVisible = false, resets options, then uses nextTick to show the new one. This creates a one-tick flash where the snackbar is invisible. In practice the transition may mask this, but if the transition is fast or skipped (e.g., in tests or reduced-motion), the user might see a flicker. Consider whether forceReuse should be the default replacement strategy, or document why the close-then-reopen is preferred.

expect(container.querySelector('.k-snackbar-backdrop')).not.toBeInTheDocument();
});
});
describe('events', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The test suite covers rendering, action button, backdrop, action-click event, and auto-hide timer — good coverage of the core behaviors. Missing coverage for: (1) Esc key triggering close event (line 32 of KSnackbar.vue), (2) backdrop focus trapping behavior, (3) focus restoration on close. These are key accessibility features worth testing.

};
}

const snackbarIsVisible = ref(false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The module-level singleton refs are consistent with how other composables in this codebase work (useKResponsiveWindow, _useKWindowDimensions). The readonly() wrapping in the return value prevents external mutation — good defensive pattern.


.k-snackbar-wrapper {
position: relative;
z-index: 24;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: The z-index values (23 for backdrop, 24 for wrapper and snackbar) appear to be hardcoded magic numbers. The existing codebase likely has z-index conventions — consider using a shared constant or commenting why these specific values were chosen.

@AlexVelezLl
Copy link
Copy Markdown
Member

Hello @AlexVelezLl ,also wanted to ask about the main navigation refractor in which i was working ,i think now the necessary kds releases are done,so ahould i start the main work there?

Yes! Sounds good.

const { sendPoliteMessage, sendAssertiveMessage } = useKLiveRegion();

const announce = message => {
if (props.backdrop) {
Copy link
Copy Markdown
Member

@MisRob MisRob Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't assume polite vs. assertive based on backdrop presence. Actually we can't even assume whether a live region announcement should be sent. Often yes, but it's not always appropriate. For example, in Studio there is one place where something like 'Channel added' snackbar appears when checking a checkbox, but the announcement should be skipped since aria-checked already communicates the state change. Utilizing live region would cause duplicate info for screen reader users, which is problematic ~ they perceive content linearly and it's important to prevent overwhelm.

So snackbar should have an option to act as an announcement, but it needs to be configurable. In addition, I believe we should actually enforce developers to always specify it explicitly since there are many situations and each need to be considered carefully.

What about something like this

setup() {
  createSnackbar({ text: ..., announce: true });
}
setup() {
  createSnackbar({ text: ..., announce: false });
}

where announce parameter is by default undefined and there is a validator that checks that true or false is provided each time (and if not, shows a warning for developers with some helpful instructions)? Also documentation would ideally have some guidance.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I thought this was mentioned somewhere in the specs 😅. This is very interesting, on one side my mind is like "we'll need more things for a simple snack bar", but on the other side, this will make us be very conscious if we want this message to be announced or not. With this, we will be dropping support for the API createSnackbar("Text"), right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes yes. I wasn't aware of this when I wrote the issue, didn't think about it in such a detail back then :)

Copy link
Copy Markdown
Member

@MisRob MisRob Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll need more things for a simple snack bar

Yes. Alternatively, we could setannounce=false by default. But since it's such a delicate decision that needs to be considered, and both having it could be very wrong and not having it could too be very wrong ~ therefore the idea to enforce explicit decision. Feel free to think it through and take the final decision. Since this is a new component ~ in terms of releases & migration, I am not too worried. We will have time to chat & plan (Studio is my primary motivation now).

Also I should have mention that createSnackbar({ text: ..., announce: true }) was meant to be polite by default (assertive is to be used rarely for critical errors). If we needed to do assertive, we could just createSnackbar({ text: ..., announce: true, assertive: true })?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like the createSnackbar({ text: ..., announce: true, assertive: true }) path. And yes, given that setting a default for this could be very wrong in either direction, I think it's reasonable to leave it without a default value and make it required :). cc: @Prashant-thakur77.

If we go with announce: true, assertive: true, though, we should also add a warning if we ever set announce: false, assertive: true just in case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also add a warning if we ever set..

Ah yes that'd be good. Good, thank you. @Prashant-thakur77 feel free to proceed with this part :)

@MisRob
Copy link
Copy Markdown
Member

MisRob commented Apr 2, 2026

Great work here @Prashant-thakur77 and @AlexVelezLl. Thank you. Hopefully I didn't miss any comment. Before proceeding with work, let me know what you think about my replies.

@Prashant-thakur77
Copy link
Copy Markdown
Contributor Author

Prashant-thakur77 commented Apr 2, 2026

Great work here @Prashant-thakur77 and @AlexVelezLl. Thank you. Hopefully I didn't miss any comment. Before proceeding with work, let me know what you think about my replies.

Sure @MisRob ,will wait for confirmation from @AlexVelezLl side.and while at it ,i will ponder on the ideas and improvements you mentioned 🤔.

@MisRob
Copy link
Copy Markdown
Member

MisRob commented Apr 2, 2026

Thanks @Prashant-thakur77. The API consideration basically simplifies the public interface for the most common cases but at the core builds on what you've already prepared, good work! I appreciated the flexibility of the loose connection between the component with the composable and am sure that's something to preserve in any case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants