Created new studioBanner for form validation by yeshwanth235 · Pull Request #5214 · learningequality/studio

yeshwanth235 · 2025-07-28T09:03:06Z

Summary

Created a new studioBanner for form validation.
Replaced Banner component with new studioBanner

Screenshots:

References

Reviewer guidance

Check storage request form validation.
If there are any errors new banner should be shown.
On change of any field. Should update error count in banner

MisRob · 2025-07-29T06:39:18Z

Thanks @yeshwanth235! High-level it is aligned with expectations. We will assign someone for a detailed review soon.

Meanwhile, would you be able to update to align with this acceptance criteria?

If there is no unit test suite, a new one is created. Do not use obsolete @vue/test-utils approach. Instead, use Vue Testing Library.

It should be a straightforward update - let us know if you had any questions.

And for accessibility, I forgot to mention it in the issue - to make error information accessible to screen readers, could you use useKLiveRegion to send a polite (recommended by our a11y lead) message to the live region when the error banner text is displayed? The message should be translated and the same as error banner text.

yeshwanth235 · 2025-07-29T07:08:23Z

Added the useKliveRegion @MisRob

One doubt regarding test cases. Instead of using @vue/test-utils. Are you suggesting to use @testing-library/vue for writing test case?

…nto issue-5082

MisRob · 2025-07-29T13:33:27Z

Yes @yeshwanth235

yeshwanth235 · 2025-07-30T18:48:52Z

updated the test cases @MisRob
please help to review

MisRob · 2025-07-31T14:21:32Z

Thanks @yeshwanth235. My good colleague @AllanOXDi will review your pull request :)

AllanOXDi

Thanks @yeshwanth235 for the great work on this! 🙌 Overall, everything looks good A few notes and suggestions - I am not sure why the error is persistent but not showing me the required field

AllanOXDi · 2025-08-07T13:23:51Z

+      },
+    },
+    watch: {
+      errorText(newText) {


This block only runs when the errorText prop changes. It does NOT run if
the error prop changes from false to true while the text stays the same. Th banner may appears with error: false and errorText: and error becomes True, but errorText doesn't change. I think we should also watch the error prop or, even better, watch a computed property that combines both error and errorText to decide when to announce

If we were to announce every error count update, then yes, this would make much sense.

However, I'd like to suggest not using watcher to not overwhelm screen reader users whose experience is different from those that are sighted. Sighted users perceive more kinds of information at once. Screen reader users get information in a linear way, one by one, and if there's too much of it, it can get difficult.

For now, instead of using watcher, let's only announce banner error text after user clicks submit button. That will be the most reasonable start. Later when we have a11y QA in Studio, we can further adjust if needed (there will likely be more work in this form anyway that's not in scope of this particular task).

Sure, will update it misRob

It seems that announcing got lost completely in the last version @yeshwanth235. Were there any issues with implementing what's suggested?

Sorry, my bad. Will update it @MisRob

No problem, thanks

AllanOXDi · 2025-08-07T13:33:24Z

+    expect(screen.getByText(sampleInfoMessage)).toBeInTheDocument();
+  });
+
+  test('error true with errorText', () => {


None blocking: it may be good to add a test for when errorText is empty or error is false to ensure the banner still renders correctly without triggering sendPoliteMessage.

AllanOXDi · 2025-08-07T13:35:39Z

+    mockSendPoliteMessage.mockClear();
+  });
+
+  test('smoke test - with default props', () => {


renaming this to something like renders banner with error text to describe behavior more explicitly.

AllanOXDi · 2025-08-07T13:44:41Z

Screen.Recording.2025-08-07.at.15.58.59.mov

yeshwanth235 · 2025-08-08T02:11:56Z

Will check it, and will push the changes.

yeshwanth235 · 2025-08-12T03:57:25Z

Screen.Recording.2025-08-07.at.15.58.59.mov

I check this issue. It is happening in main branch as well. Can you please help to create an issue. @AllanOXDi

The validation error is now showing for this field

MisRob · 2025-08-13T15:17:26Z

Hi @yeshwanth235 and @AllanOXDi - I wanted to note that after it's re-reviewed, let's wait for merging until I let you know. This is due to pre-release preparations, and will take only a couple of days.

Yes @yeshwanth235, you don't need to fix something that was present on unstable before and reporting as a new issue together with @AllanOXDi will be best.

MisRob · 2025-08-22T15:04:03Z

@AllanOXDi @yeshwanth235 I was just notified, we can merge PRs now. Allan, do you have any other feedback?

MisRob · 2025-08-22T15:04:41Z

+<template>
+
+  <div
+    class="banner notranslate"


Why is notranslate here?

Since div contains text. Hence kept the class.

notranslate is only for some user-generated texts. To prevent Google Translate from translating it.

So here can be removed.

It's a bit nuanced. Generally when working on Studio, you can remember that for anything that's application text, we don't want to prevent translation.

One of my colleagues recently summed it up nicely. Hopefully it helps to understand.

Studio's UI isn't translated into as many languages as Kolibri, but being an online service, we've supported using in-browser Google Translate as an option for those unsupported languages. The notranslate class then tries to align that behavior like it was a UI language option-- only translating Studio's UI and not user-generated text.

AllanOXDi · 2025-08-22T15:04:43Z

Let me re review it

MisRob · 2025-08-22T15:06:28Z

+
+    const banner = screen.getByTestId('studio-banner');
+    expect(banner).toBeInTheDocument();
+    expect(banner).toHaveClass('banner', 'notranslate');


There's no need to test for presence of classes - it doesn't really say that much about resulting user experience, and also ties tests to internal implementation.

yeshwanth235 · 2025-08-22T16:06:31Z

Thanks for review @MisRob

Updated the PR. Please help to review @AllanOXDi and @MisRob

MisRob

Thanks @yeshwanth235, two last notes from me. Unless @AllanOXDi has some feedback, I believe we can then merge.

MisRob · 2025-08-27T14:51:07Z

  }

+  .studio-banner {
+    margin-top: 8px !important;


Why was !important needed?

Will check and update it

MisRob · 2025-08-27T15:04:13Z

+      const slotContent = this.$slots.default;
+      const textContent = slotContent[0].text;
+      if (textContent && this.error) {
+        this.sendPoliteMessage(textContent);


Almost there :) Screen reader now properly announces the error message when I click "Send request" for the first time, while not announcing each dynamic change to error count as I requested 👍

However when I fix few errors, but not all of them, and click the button again, it doesn't announce. This is because the same banner is displayed, and mounted is not executed again. Maybe we could simply sendPoliteMessage in Send request click handler, outside StudioBanner component? Since StudioBanner itself doesn't really have enough context to distinguish between what's dynamic error count updates and what's the error message when the form is clicked again.

Made changes @MisRob. Thanks for the suggestion.

Now sendPoliteMessage will be called in onValidationFailed. Which is triggered by form validation. So from now on when ever the submit is click. If there are any errors sendPoliteMessage will be sent.

AllanOXDi

LGTM! , Will differ to @MisRob for the final review

MisRob

Thanks @yeshwanth235 @AllanOXDi.

Nice work. Screen reader now reports in a balanced manner 👍

Allan approved above

yeshwanth235 and others added 4 commits July 28, 2025 14:06

Created new studioBanner for form validation

90ec7a5

[pre-commit.ci lite] apply automatic fixes

7d56b91

small change

a88acad

[pre-commit.ci lite] apply automatic fixes

4b5aac0

yeshwanth235 and others added 4 commits July 29, 2025 14:39

added kLiveRegion

c2febf8

[pre-commit.ci lite] apply automatic fixes

e6dbc22

small change

b389537

Merge branch 'issue-5082' of https://github.com/yeshwanth235/studio i…

53e139e

…nto issue-5082

rtibbles assigned AllanOXDi Jul 29, 2025

updated the test ccases and logic

d27a23e

[pre-commit.ci lite] apply automatic fixes

d9b1a8e

AllanOXDi self-requested a review August 5, 2025 11:20

AllanOXDi previously requested changes Aug 7, 2025

View reviewed changes

yeshwanth235 and others added 2 commits August 12, 2025 09:23

addressing PR comments

ad877f9

[pre-commit.ci lite] apply automatic fixes

d713285

marcellamaki modified the milestones: Studio Q2 Patches, Studio: search recommendations release Aug 12, 2025

MisRob requested a review from AllanOXDi August 13, 2025 09:47

bjester modified the milestones: Studio: search recommendations release, Studio: Q3 (patch) release Aug 13, 2025

MisRob reviewed Aug 22, 2025

View reviewed changes

addressing PR comments

0c3f925

MisRob reviewed Aug 27, 2025

View reviewed changes

addressing pr comments

f4d9955

AllanOXDi reviewed Aug 29, 2025

View reviewed changes

MisRob approved these changes Sep 3, 2025

View reviewed changes

MisRob merged commit c211626 into learningequality:unstable Sep 3, 2025
13 checks passed

MisRob mentioned this pull request Sep 3, 2025

[Remove Vuetify from Studio] 'Request more space' form error banner in Settings - Storage #5082

Closed

11 tasks

Uh oh!

Conversation

yeshwanth235 commented Jul 28, 2025

Summary

References

Reviewer guidance

Uh oh!

MisRob commented Jul 29, 2025

Uh oh!

yeshwanth235 commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MisRob commented Jul 29, 2025

Uh oh!

yeshwanth235 commented Jul 30, 2025

Uh oh!

MisRob commented Jul 31, 2025

Uh oh!

AllanOXDi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AllanOXDi Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MisRob Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AllanOXDi Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AllanOXDi commented Aug 7, 2025

Uh oh!

yeshwanth235 commented Aug 8, 2025

Uh oh!

yeshwanth235 commented Aug 12, 2025

Uh oh!

MisRob commented Aug 13, 2025

Uh oh!

MisRob commented Aug 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AllanOXDi commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MisRob Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeshwanth235 commented Aug 22, 2025

Uh oh!

MisRob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MisRob Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

yeshwanth235 commented Jul 29, 2025 •

edited

Loading

AllanOXDi left a comment •

edited

Loading

AllanOXDi Aug 7, 2025 •

edited

Loading

MisRob Aug 8, 2025 •

edited

Loading

AllanOXDi Aug 7, 2025 •

edited

Loading

AllanOXDi commented Aug 22, 2025 •

edited

Loading

MisRob Aug 22, 2025 •

edited

Loading

MisRob Aug 27, 2025 •

edited

Loading

yeshwanth235 Aug 29, 2025 •

edited

Loading