Page MenuHomePhabricator

Create a standalone Thumbnail component
Closed, ResolvedPublic

Description

Summary

The MenuItem component contains the following thumbnail behavior and logic:

  • Display a thumbnail image as a background image
  • If there is no thumbnail image, or if there is but it's still loading, display a placeholder icon

The Card component will need the same features, so we should extract this from MenuItem into a new Thumbnail component.

Design spec

Considerations

  • Some styles may be shared by both MenuItem and Card, and should therefore be included in the new Thumbnail component. Others may be more specific to the component implementing Thumbnail, and should reside in those components.

Acceptance criteria

  • If needed, a Figma spec for Thumbnail is created
  • A new Thumbnail component is added to Codex
  • The Thumbnail component is implemented by MenuItem (this task should be completed before the Card MVP, so using Thumbnail within Card is not part of this task's scope)
  • Any styles specific to MenuItem will remain in the MenuItem component

Event Timeline

Change 809291 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] Thumbnail: Add Thumbnail component

https://gerrit.wikimedia.org/r/809291

Change 809292 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] MenuItem: Use the Thumbnail component

https://gerrit.wikimedia.org/r/809292

Moved to the sprint board because this must precede the Card MVP.

@Sarai-WMDE No rush on this since TypeaheadSearch is the priority, but when you have time, this is ready for your review. Check out the new Thumbnail component and the updated MenuItem component (which should look and behave exactly the same), and TypeaheadSearch for good measure.

I moved all thumbnail styles into the Thumbnail component except for the margin-right that's used within MenuItem, which is specific to Menuitem.

Hey @AnneT 👋🏻 The Thumbnail component is looking great. I didn't notice any changes (visual or otherwise) in the MenuItem or TypeaheadSearch components.

If that's ok, I will review the component again in more depth once the patch is merged/ready to be merged. I'm not sure if there are any changes that still need to be applied.

Btw, I just replaced the link to the specs in the task description. Exploration file should be considered source of truth during implementation, instead of the library specs: these shouldn't have been there, sorry for the confusion. I deleted the component page from the public Figma library for now.

After looking at the current implementation, I think that the main points to discuss (and, in any case, cover in follow-up tasks) would be sizing and aspect ratio. This probably deserves a dedicated conversation, but I thought I could provide some thoughts in preparation for that.

Sizing

We should consider applying relative sizes to thumbnails. This way, they would also proportionally adjust whenever the font size of the content they accompany is modified. I, so far, considered the following options:

  1. Creating some premade sizes: For example S (2.5em), used in the context of MenuItem; and M (3em), used in landscape oriented cards.
  2. Making thumbnails 100% in width and height by default, and allowing implementers (and ourselves) to decide their height and width in context via a wrapper. (This might be quite a more flexible option)

When it comes to the thumbnail placeholder icon, I believe that this should also be relative to the size of its container (the thumbnail). Particularly 50%.
Another requirement is to set a max-width and max-height to the icon, which should never be bigger than 64px (to be replaced by the right size token).

Aspect ratio

Thumbnails are not always square. They might have an aspect ratio of 16:9 instead (e.g. like in portrait-oriented cards). I'm not sure if we should just let this different proportions to be defined in context (e.g. by making it possible to resize thumbnails, following the second option listed above), or whether we should implement this as a prop. Again, the former sounds more flexible.

On their side, thumbnail placeholder icons should always maintain a 1:1 aspect ratio, and not be "deformed" in case the proportions of the thumbnail change.

This blocks Card component

Thanks for the clear writeup, @Sarai-WMDE! It sounds like we should open separate tasks for these new features, and complete them in the order you listed them (relative sizing, then configurable sizing, then configurable aspect ratio).

Shall we open 3 separate tasks, then meet sometime soon to discuss the implementation details? I have some questions about how to handle aspect ratio, and relative sizing opens up a big can of worms related to TypeaheadSearch and all of the styles in both Codex and Vector that are based on the current absolute size of the thumbnail (40px).

Happy to meet whenever possible. Until then, I finally went ahead and created two "needs discussion" tasks:

I still need to make some design verifications first, but we might not need to take care of aspect ratio yet because this property only applied to "thumbnails" included within portrait-oriented Cards. I realized that, due to their size and rectangular proportions, these pieces of media could be simply considered images instead.

Regarding relative sizing: this is actually the approach taken by Vector, which contributed to my suggestion.

Screenshot 2022-07-07 at 21.12.36.png (1×2 px, 1 MB)

(Now that's a historically relevant screenshot!)

Change 809291 merged by jenkins-bot:

[design/codex@main] Thumbnail: Add Thumbnail component

https://gerrit.wikimedia.org/r/809291

Change 809292 merged by jenkins-bot:

[design/codex@main] MenuItem: Use the Thumbnail component

https://gerrit.wikimedia.org/r/809292

@Sarai-WMDE awesome, thanks for opening those tickets! I'll take a closer look at them after inspiration week and we can continue collaborating there :)

Change 812107 had a related patch set uploaded (by DannyS712; author: DannyS712):

[design/codex@main] Thumbnail: clean up testThumbnail in tests

https://gerrit.wikimedia.org/r/812107

Change 812107 merged by jenkins-bot:

[design/codex@main] Thumbnail: clean up testThumbnail in tests

https://gerrit.wikimedia.org/r/812107

The new standalone Thumbnail component can be signed off from a design point of view. The component matches the early design specifications, and its extraction didn't introduce any changes (visual or otherwise) in the MenuItem or TypeaheadSearch components. The review was performed in Safari 15.5, Firefox 102 and Chrome 103 in MacOS Monterey.

Just two questions:

  • Should we showcase the placeholderIcon prop in a configuration section? If this makes sense, I'll gladly create a task.
  • Also, in the demo page, maybe we should refer to the placeholder variant as "Placeholder" instead of placeholder icon? I'm referring to these bits of text only:

Screenshot 2022-07-20 at 12.55.07.png (276×674 px, 39 KB)

Further modifications will be applied (if agreed after needed discussions) in follow-up tasks:

T312580: [Needs discussion] Thumbnail should be resizeable
T312582: Thumbnail: Thumbnails should have relative sizes
T313214: Thumbnail: Replace default placeholder icon

Status update: I pinged Anne regarding the comment above, which collects non-blocking questions and suggestions.

Change 823744 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] docs, Thumbnail: Update "placeholder" language

https://gerrit.wikimedia.org/r/823744

@Sarai-WMDE Thanks for the suggestions around the placeholder language—I've pushed a patch to change "placeholder icon" to "placeholder" in the docs, and to add a sentence to the demo for the custom placeholder icon to clarify that part of the config a bit. You can check it out here—let me know what you think!

Thanks for the changes, @AnneT. I'll create a task to include a configurable version of the Thumbnail component in the demo page. Let me know if that makes sense! Signing-off.

Change 823744 merged by jenkins-bot:

[design/codex@main] docs, Thumbnail: Update "placeholder" language

https://gerrit.wikimedia.org/r/823744

Looks good to me. I also tested this component on TAhS and when using a slow internet connection.

Moving to Product Sign-off.

Change 828083 had a related patch set uploaded (by Catrope; author: Catrope):

[mediawiki/core@master] Update Codex from v0.1.0-alpha.10 to v0.1.0

https://gerrit.wikimedia.org/r/828083

Change 828083 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.1.0-alpha.10 to v0.1.1

https://gerrit.wikimedia.org/r/828083

DAbad changed the task status from Open to In Progress.Sep 1 2022, 2:50 PM
DAbad subscribed.

Product sign-off complete