[Remove Vuetify from Studio] Cards in Starred channels changes #5556
Conversation
MisRob
left a comment
MisRob
left a comment
There was a problem hiding this comment.
Very nice work, @yeshwanth235, thanks a lot.
In testing, all worked smoothly! There's only one smaller issue with focus ring, and also markup structure needs to be adjusted at once place. Other notes are really just cleanup or organizational improvements.
As soon as you continue with View-only, please use the same approach. There's no need to worry about those few remaining markup duplicates. I'd like us to finish the current issues at first, and then I will plan remaining work - it will be easier after all main pieces are in place.
|
|
||
| const loadData = async () => { | ||
| loading.value = true; | ||
| try { |
There was a problem hiding this comment.
I haven't tried, just from how the implementation is done - it seems that if there was an error when loading a channel list, it would fail silently? We don't need any new UI, mostly wondering what would experience be like and how it compares to the original version (e.g. was there a console error displayed before or not,...)?
There was a problem hiding this comment.
In previous version. There is no catch block.
L151
| } | ||
| }; | ||
|
|
||
| const newChannel = () => { |
There was a problem hiding this comment.
Since the logic for creating a new channel is specific only to StudioMyChannels and none of the other pages, I think it'd make more sense to have this piece in StudioMyChannels's setup() rather than in the composable.
|
|
||
| loadData, | ||
| newChannel, | ||
| goToChannel, |
There was a problem hiding this comment.
I haven't noticed goToChannel method to be imported from any place. Should it be removed then?
This applies to few other state/computed/methods - some of them seem to be unused.
| }; | ||
| }, | ||
| data() { | ||
| // Use the channel list composable |
There was a problem hiding this comment.
The comment can be cleaned up - to those familiar with composables, code is self-explanatory.
| min-width: 24px; | ||
| height: 50%; | ||
| } | ||
| @import './styles/StudioChannels'; |
There was a problem hiding this comment.
Nice, thank you for abstracting the styles. This will help us later on with some final components I am planning.
| :ref="`detailIcon${index}`" | ||
| class="details-link" | ||
| > | ||
| <router-link |
There was a problem hiding this comment.
I can't see the blue focus ring around the icon link. Is ':focus': { ...this.$coreOutline, outlineOffset: 0 }, missing? See the example here for full code https://design-system.learningequality.org/kcard#interactive-elements
| type: Object, | ||
| required: true, | ||
| }, | ||
| index: { |
There was a problem hiding this comment.
Since a single card is now abstracted to a component, index can be removed. After you've re-organized test suites as suggested in the other comment, it won't be needed - e.g. instead of :data-testid="dropdown-button-${index}", you can just :data-testid="dropdown-button". And instead of :ref="detailIcon${index}", you too can just :ref="detailIcon" - references are be scoped to a component.
| return { | ||
| tokenDialog: false, | ||
| deleteDialog: false, | ||
| selectedChannel: { |
There was a problem hiding this comment.
Since now this components represents a single channel/card, all the places utilizing selectedChannel can be updated to simply use the channel coming from the channel prop, and selectedChannel can be removed.
| }; | ||
| }, | ||
| computed: { | ||
| // channel items properties |
| width: 100%; | ||
| } | ||
| .cards-below-title { |
There was a problem hiding this comment.
It'd be meaningful to remove .cards- prefix from this and other classes - since now we're inside the card component.
2 participants
Summary
Screen.Recording.2025-11-17.at.11.27.12.AM.mov
References
5524
Reviewer guidance