-
Notifications
You must be signed in to change notification settings - Fork 251
[Remove Vuetify from Studio] Channel details in Channels - content #5540
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: unstable
Are you sure you want to change the base?
[Remove Vuetify from Studio] Channel details in Channels - content #5540
Conversation
61e0727 to
5c41386
Compare
|
Hi @vtushar06, thank you. As for the structure of components, and very high-level, looks as expected 👍 Before we dive into full review though, let's revisit approach to testing. This will help a reviewer too - they will be able to recognize whether the logic behaves as expected for channels with/without all sorts of data. Also, the guidance I left is quite general. I know you have more PRs open - would you please check if there are some similar patterns to be addressed there as well? |
| expect(container2.querySelector('.studio-chip')).toHaveTextContent('Chip 2'); | ||
| }); | ||
|
|
||
| it('has correct styling classes', () => { |
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.
Can be removed.
We typically don't test styling or classes in unit tests - it doesn't say that much about resulting experience. And is extra maintenance - when class or style is removed or renamed, we'd need to update test.
Generally in all our tests you can focus on testing (1) rendered content, (2) business logic. Please revisit other tests too.
| expect(container.querySelector('.v-chip')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('does not use VLayout or VFlex', () => { |
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.
Users are not interested in our Vuetify refactor - for them it doesn't matter if we use VFlex or flex style. And after Vuetify is removed from Studio, what would be value of testing this?
Would you find some articles around good unit testing practices, and also have a look at VTL guiding principles? For each test scenario you write, reflect critically on why is the scenario useful or not, and what it tests from the user point of view.
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.
This test suite needs to test things like
- If a channel has levels, a corresponding text is rendered
- If a channel has no levels, --- is rendered
This is just one of many examples. Same applies for all other information on the page - preview it as a user for channel with/without data, and then make unit tests to document the behavior.
Summary
Migrated ChannelDetailsModal from Vuetify to Kolibri Design System (KDS) by creating three new reusable components:
New Components:
StudioChip: Replaces VChip for displaying tags and metadata with consistent rounded styling
StudioDetailsRow: Replaces VLayout/VFlex with CSS Grid for semantic detail rows with label/value pairs
StudioDetailsPanel: Complete KDS rewrite of DetailsPanel using KImg, KTooltip,semantic
lists- Replaced Vuetify components (VCard, VLayout, VFlex, VChip, VTooltip, VDataTable) with KDS equivalents
- Used CSS Grid and Flexbox for all layouts
- Implemented KImg with 16:9 aspect ratio for channel thumbnails
- Added comprehensive unit tests (19 tests passing) using @testing-library/vue
- Check RTL language support
- Run unit tests: npm test StudioChip StudioDetailsRow StudioDetailsPanel
Key Changes:
Screen-Recording:
Channel.details.modal.mp4
…
References
fixes : #5530
…
Reviewer guidance
…