Cards in My Channels

[yeshwanth235]

Summary

Created StudioMyChannels component.
Which uses KCardGrid to show cards
KDropdownMenu used for dropdown.
KModal is used for delete and copy features.
Updated /my-channels component to StudioMyChannels

Video

References

5227

Reviewer guidance

Need to end to end testing for /my-channels.

[MisRob]

Thank you @yeshwanth235! I set aside some time this week to review everything.

[MisRob]

(1) Can you ensure that card width is same as before on all resolutions? I would recommend you create a wrapper container that has a grid and the button in it, set paddings in the wrapper container, and then let the grid take 100% width of the wrapper. If it helps, a convenient way to preview is to star some channels and then switch between the new My Channels and the Starred Channels that still has the previous version, and ensure that My Channels results in same layout experience.

(2) There's missing space above the button.

(3) Would you follow this guidance to display the thumbnail placeholder? Including placeholder icon responsive behavior (guidance). Again compare with the previous version which has a gray image icon when thumbnail image is not available - that needs to be preserved - including how it scales size depending on card size.

[MisRob]

Would you remove listType prop and all related conditional logic from StudioMyChannels?StudioMyChannels will always be EDITABLE - so you can just assume it in that component. I've tried to say more in the issue, not sure if it's explained well - let me now if you had any questions.

Note listType prop removal from RouteNames.CHANNELS_EDITABLE route above. In contrast to shared ChannelList that is re-used from multiple pages and requires related conditional logic, StudioMyChannels will be only used from My Channels. Therefore, do not add listType prop to StudioMyChannels. Preserve only logic that was previously related to ChannelListTypes.EDITABLE type. Cleanup copied code to not contain listType conditions and code for other types, such as ChannelListTypes.PUBLIC.

Otherwise, as we progress refactoring other tabs on the page, StudioMyChannels would end up as a monolithic component with lots of conditional logic and hard to maintain. The current ChannelList suffers from this problem and I want us to avoid it in new components. We will take a different approach to re-use logic - I will outline the strategy in the upcoming issues.

[yeshwanth235]

Need some clarification

So for starred and view-only pages. Are we going to use same component StudioMyChannels or it will be an new component

[MisRob]

v-if="!listChannels || !listChannels.length" would be a bit more robust.

Also let's adjust the condition to make sure that "No channels found" is not displayed while channels are loading. Otherwise, particularly on slower network, we will see "No channels found" just to be replaced by channels in a moment.

[MisRob]

Could you use skeletonsConfig to

• (1) Show 3 skeleton cards
• (2) For each breakpoint, configure skeleton to resemble loaded cards as closely as possible

There's quite a detailed tutorial here. Notice debug prop that will help you to fine-tune for each breakpoint.

I was able to preview loading state when I turned on network throttling 2G:

But for development, I think it'd be most convenient if you just set loading to true temporarily :)

[MisRob]

We avoid hardcoded colors in our codebase in favor of KDS theme colors. See the same page for why.

For black text, we'd want to use tokens.text.

[yeshwanth235]

updated it.

[MisRob]

And here tokens.annotation.

[yeshwanth235]

updated it.

[MisRob]

Why was this needed?

I know ::v-deep is used in the codebase, however it's an anti-pattern we're trying to get rid of. Here's on why.

If there's some limitation in KDS that caused the need for it, let me know and I will help with next steps :)

[yeshwanth235]

This class is not required removed it.

[MisRob]

This can be removed - RTLCSS library takes care of it - however it can only be previewed on the devserver without :hot

pnpm run devserver

I've just double-checked and all is fine without it.

[yeshwanth235]

will check this and update it

[yeshwanth235]

Removed it. Working fine in npm run devserver

[MisRob]

Also can be removed. KDS and RTLCSS takes care of this - see the related comment for more.

[yeshwanth235]

Removed it

[MisRob]

Alongside similar comments, libraryMode will never be truthy on StudioMyChannels so it can be removed from here and elsewhere in the file (+ I've just learned that it's an obsolete mode that doesn't take effect in the current version either)

[MisRob]

I was wondering how these /locale changes found their way in here?

In any case, these files will need to be removed from the PR - they will be prepared later as part of the release translation process.

[yeshwanth235]

I have added them. As mentioned it will be prepared while release. Hence removing them

[yeshwanth235]

removed the locale changes.

[MisRob]

Hi @yeshwanth235, note that the amount of comments I am leaving here is very common in my reviews for larger PRs :) Overall this is fantastic work and I'm really happy you picked one of the high complexity issues - you've handled it very well. It is aligned with what was requested in the issue, and all main pieces work great in this very first version. Thanks a lot.

Most of my feedback is around few areas where I noticed some regression. Also we can simplify some conditional logic. Take all the time you need, and let me know if you had any questions.

[yeshwanth235]

Thanks for the review @MisRob
Will address the comments

[MisRob]

Thanks @yeshwanth235. I wanted to mention that I will be on vacation until September 22. My colleague @rtibbles may leave some notes as well meanwhile, and I will get back to you as soon as I'm back :)