-
-
Notifications
You must be signed in to change notification settings - Fork 303
Created new studioBanner for form validation #5214
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
Changes from 10 commits
90ec7a5
7d56b91
a88acad
4b5aac0
c2febf8
e6dbc22
b389537
53e139e
d27a23e
d9b1a8e
ad877f9
d713285
0c3f925
f4d9955
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 |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| <template> | ||
|
|
||
| <div | ||
| class="banner notranslate" | ||
| 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) { | ||
|
Contributor
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. This block only runs when the
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. 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).
Contributor
Author
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. Sure, will update it misRob
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. It seems that announcing got lost completely in the last version @yeshwanth235. Were there any issues with implementing what's suggested?
Contributor
Author
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. Sorry, my bad. Will update it @MisRob
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. No problem, thanks |
||
| if (newText.length && this.error) { | ||
| this.sendPoliteMessage(newText); | ||
| } | ||
| }, | ||
| }, | ||
| mounted() { | ||
| if (this.errorText.length && this.error) { | ||
|
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', () => { | ||
|
Contributor
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. renaming this to something like |
||
| renderComponent({ errorText: sampleInfoMessage }); | ||
|
|
||
| const banner = screen.getByTestId('studio-banner'); | ||
| expect(banner).toBeInTheDocument(); | ||
| expect(banner).toHaveClass('banner', 'notranslate'); | ||
|
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. 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', () => { | ||
|
Contributor
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. 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); | ||
| }); | ||
| }); | ||
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.
Why is
notranslatehere?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.
Since div contains text. Hence kept the class.
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.
notranslateis 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.
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.
One of my colleagues recently summed it up nicely. Hopefully it helps to understand.