Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
ref="form"
@submit.prevent="submit"
>
<Banner
:value="Boolean(errorCount())"
error
:text="errorText()"
<StudioBanner
v-if="Boolean(errorCount())"
class="my-2"
error
:errorText="errorText()"
/>

<!-- Nature of content -->
Expand Down Expand Up @@ -268,7 +268,7 @@
import { LicensesList } from 'shared/leUtils/Licenses';
import CountryField from 'shared/views/form/CountryField';
import MultiSelect from 'shared/views/form/MultiSelect';
import Banner from 'shared/views/Banner';
import StudioBanner from 'shared/views/StudioBanner';
import InfoModal from 'shared/views/InfoModal';

const formMixin = generateFormMixin({
Expand Down Expand Up @@ -320,7 +320,7 @@
components: {
CountryField,
MultiSelect,
Banner,
StudioBanner,
InfoModal,
},
mixins: [constantsTranslationMixin, formMixin],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<template>

<div
class="banner notranslate"
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.

Why is notranslate here?

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.

Since div contains text. Hence kept the class.

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.

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.

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.

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.

data-testid="studio-banner"
:style="{ backgroundColor: error ? $themePalette.red.v_100 : '' }"
>
{{ errorText }}
</div>

</template>


<script>

import useKLiveRegion from 'kolibri-design-system/lib/composables/useKLiveRegion';

export default {
name: 'StudioBanner',
setup() {
const { sendPoliteMessage } = useKLiveRegion();
return { sendPoliteMessage };
},
props: {
error: {
type: Boolean,
default: false,
},
errorText: {
type: String,
default: '',
},
},
watch: {
errorText(newText) {
Copy link
Copy Markdown
Contributor

@AllanOXDi AllanOXDi Aug 7, 2025

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@MisRob MisRob Aug 8, 2025

Choose a reason for hiding this comment

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

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).

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.

Sure, will update it misRob

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.

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

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.

Sorry, my bad. Will update it @MisRob

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.

No problem, thanks

if (newText.length && this.error) {
this.sendPoliteMessage(newText);
}
},
},
mounted() {
if (this.errorText.length && this.error) {
Comment thread
MisRob marked this conversation as resolved.
Outdated
this.sendPoliteMessage(this.errorText);
}
},
};

</script>


<style lang="scss" scoped>

.banner {
padding: 16px;
}

</style>
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { render, screen } from '@testing-library/vue';
import VueRouter from 'vue-router';
import StudioBanner from '../StudioBanner.vue';

// Mock the Kolibri design system composable
const mockSendPoliteMessage = jest.fn();
jest.mock('kolibri-design-system/lib/composables/useKLiveRegion', () => ({
__esModule: true,
default: () => ({
sendPoliteMessage: mockSendPoliteMessage,
}),
}));

const sampleErrorMessage = 'This is an error message';
const sampleInfoMessage = 'This is an info message';

// Helper function to render the component with the provided props
const renderComponent = (props = {}) =>
render(StudioBanner, {
props: {
error: false,
errorText: '',
...props,
},
routes: new VueRouter(),
});

describe('StudioBanner', () => {
beforeEach(() => {
mockSendPoliteMessage.mockClear();
});

test('smoke test - with default props', () => {
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.

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

renderComponent({ errorText: sampleInfoMessage });

const banner = screen.getByTestId('studio-banner');
expect(banner).toBeInTheDocument();
expect(banner).toHaveClass('banner', 'notranslate');
Copy link
Copy Markdown
Member

@MisRob MisRob Aug 22, 2025

Choose a reason for hiding this comment

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

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.

expect(screen.getByText(sampleInfoMessage)).toBeInTheDocument();
});

test('error true with errorText', () => {
Copy link
Copy Markdown
Contributor

@AllanOXDi AllanOXDi Aug 7, 2025

Choose a reason for hiding this comment

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

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.

renderComponent({
error: true,
errorText: sampleErrorMessage,
});

const banner = screen.getByTestId('studio-banner');
expect(banner).toHaveStyle('background-color: rgb(255, 217, 211)');
expect(screen.getByText(sampleErrorMessage)).toBeInTheDocument();
expect(mockSendPoliteMessage).toHaveBeenCalledWith(sampleErrorMessage);
});
});