Skip to content

Conversation

ozer550
Copy link
Member

@ozer550 ozer550 commented Jun 25, 2025

Summary

  • Fix regressions after removing the SVG wrapper around KIcons.
  • Add utility component to support contentKindIcon functionality.

References

closes #4991

Reviewer guidance

  • We would want to regression test Thumbnail component and see if things do not break.

@ozer550 ozer550 requested a review from MisRob June 25, 2025 06:40
@MisRob MisRob self-assigned this Jun 30, 2025
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Hi @ozer550, thank you. Looks like a good start.

Next step would be to improve thumbnail icon scaling logic. I think the goal of the issue was to remove the extra <svg>, but without any changes to user experience. However as of now, icon scaling is lost - for example, if I gradually resize /channels browser window, the thumbnail icon behaves quite differently from before. Here's few places, but it affects almost every screen size.

Screen width Before / expected After
600px Screenshot from 2025-06-30 08-47-01 Screenshot from 2025-06-30 08-45-03
1380px Screenshot from 2025-06-30 08-56-56 Screenshot from 2025-06-30 08-56-38

Looking into code, there seemed to be lots of effort into implementing smooth scaling experience:

  $svg-scale: 1.25;
  $svg-width: calc(100% * 9 / 16 / #{$svg-scale});
  $svg-top: calc((100% * 9 / 16 / 2) - ($svg-width / 2));

  svg.thumbnail-image {
    top: 0;
    left: calc(50% - (#{$svg-width} / 4));
    width: calc(#{$svg-width} / 4);
   ...

Even though we want to remove extra svg, I don't think it was intended to get rid of this logic completely. We'd rather want to reapply it in a way that works with KIcon.

return icon ? CONTENT_KIND_ICONS[icon] : 'error';
}

export function getLearningActivityIcon(activity) {
Copy link
Member

Choose a reason for hiding this comment

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

This function doesn't seem to be called from anywhere. Can be removed.

@MisRob
Copy link
Member

MisRob commented Jul 1, 2025

@ozer550 I wanted to note that @akolson made some fixes to svg scaling here. It'd be probably best to wait until we merge that and then rebase this PR before we proceed with further refactors.

@rtibbles
Copy link
Member

#5147 has now been merged, so can rebase here.

@MisRob
Copy link
Member

MisRob commented Jul 28, 2025

To sum up our co-hack:

  • Applying current calculations on KIcon is not as straightforward as I expected
  • @ozer550 will explore if his flex approach can be adjusted so visually there's no regressions
  • If we have trouble with that, together we will think if there are ways to adjust KIcon to allow for easier scaling

@ozer550 ozer550 requested a review from MisRob August 20, 2025 17:24
@MisRob
Copy link
Member

MisRob commented Aug 27, 2025

Hi @ozer550, overall looking good, and I like the way it's simpler now. I previewed both compact and non-compact mode and haven't noticed any regressions. Nice work. Just two clarifications.

@@ -6,11 +6,12 @@
[kind]: compact,
'icon-only': compact,
nothumbnail: !showThumbnail && !compact,
'with-caption': showCaption,
Copy link
Member

Choose a reason for hiding this comment

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

Where can I preview before / after of thumbnails with captions?

$aspect-ratio: 9 / 16;

$aspect-percentage: $aspect-ratio * 100%;
Copy link
Member

Choose a reason for hiding this comment

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

Even though it's referenced from .thumbnail, is that style actually taking effect? Seems like something that was closely interconnected with the calculations we're now removing. So just double-checking if this is something to be cleaned up as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double wrapped thumbnail icon SVG
5 participants