Kuwait Theme: Module List Item Component

[FaithDaka]

PR Checklist

• PR title descriptive (can be used in release notes)

Description

plh_module_list_item created and styled. Corresponding debug sheet: Link

Author Notes

plh_module_list_item component with the following parameter_list:

• module_image_asset: The image attached to the module item.
• text_transform (optional): The format of the text on the module item. Default null- nav_image_asset: For the navigation icon
• locked_image_asset (optional): For the locked icon
• is_locked (optional): The boolean that marks the module as locked or unlocked

Git Issues

Closes #2534

Screenshots

Design	LTR Style	RTL Style

[github-actions]

Visit the preview URL for this PR (updated for commit 417a9f2):

https://plh-teens-app1--pr2527-feat-module-list-com-7i7exiev.web.app

_{(expires Fri, 06 Dec 2024 14:15:35 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: e4c0bab6b08dd290fbf002fd6e07987fa4b5fce1}

[esmeetewinkel]

Looks great! A few comments:

1. Should the module_alignment automatically adapt when a right-to-left language is selected? Currently it seems that the relative position of the elements within the item does flip, but the text alignment doesn't (FYI can test this behaviour by going to the template feature_rtl_language and selecting English RTL). We would also need to flip the nav_image_asset.

English regular vs English RTL:

2. Instead of a target_template parameter, should we have this in the action_list column as click | go_to: [target_template] which is more in line with our usual syntax for button actions? This would give flexibility of authoring more button actions if needed, such as setting fields / dynamic data.

3. I noticed that only the area around the nav_image_asset in this component is clickable, not the full "button". I kind of expected that the full button would be clickable (because it has a drop shadow) - but don't feel super strongly, so happy to hear what others think.

[esmeetewinkel]

@FaithDaka Which Title Case convention is used for the text-transform: capitalise parameter? I'm wondering if this will always work well with translations, or that it would be safer to send text with its capitalisation to translators so that they can assess what is appropriate in their language.

[FaithDaka]

Looks great! A few comments:

1. Should the module_alignment automatically adapt when a right-to-left language is selected? Currently it seems that the relative position of the elements within the item does flip, but the text alignment doesn't (FYI can test this behaviour by going to the template feature_rtl_language and selecting English RTL). We would also need to flip the nav_image_asset.

English regular vs English RTL:

@esmeetewinkel Currently I'm not familiar with the RTL pipe/service for components so I'd decided to start off with an alignment variable and flip using CSS, but I will go through the code to see how the pipe is used and refactor.

2. Instead of a target_template parameter, should we have this in the action_list column as click | go_to: [target_template] which is more in line with our usual syntax for button actions? This would give flexibility of authoring more button actions if needed, such as setting fields / dynamic data.

This makes sense to me. I'm still getting accustomed to the functionality in the sheets so I can switch to that.

3. I noticed that only the area around the nav_image_asset in this component is clickable, not the full "button". I kind of expected that the full button would be clickable (because it has a drop shadow) - but don't feel super strongly, so happy to hear what others think.

Great point! For a better UX the entire card should be clickable with the arrow as a visual cue. It does get messy with accidental clicks when scrolling involved but I agree that it is more intuitive.

[FaithDaka]

@FaithDaka Which Title Case convention is used for the text-transform: capitalise parameter? I'm wondering if this will always work well with translations, or that it would be safer to send text with its capitalisation to translators so that they can assess what is appropriate in their language.

@esmeetewinkel capitalise would be the Title case style rather than Headline case.
There's no default transformation for the module-value so if the text-transform is not specified, the default is none (what you type is what is rendered).
Unicameral Scripts (like Arabic) don't have case distinction and wouldn't be affected by any text-transform value therefore this variable is more to add flexibility on the author's end incase this component is to be used in a different use-case or country.

[FaithDaka]

@chrismclarke @jfmcquade Are there any concerns with this PR?

[jfmcquade]

Thanks, @FaithDaka, sorry for the slow response. I think this looks good, nice to see our component parameter conventions being used clearly.

I've added a couple of minor comments inline. I'm happy to approve but I would suggest waiting for an approval from all reviewers before merging.

[jfmcquade]

I think we probably don't need the capitalise option. It's going to apply a sub-optimal version of title case where words like "of", "to", "and" etc. get capitalised, a convention which may also vary by language. So I think capitalisation within strings is something that authors need to determine themselves when supplying the string. The same argument could be applied, to some extent, to "uppercase" too, but I can see how this could be useful so I'm happy to have it included.

[jfmcquade]

Could this be removed as an explicit property and whether or not to show the nav image just be inferred from whether or not an asset is provided?

[FaithDaka]

@jfmcquade I'll use the is-locked boolean property given that navItem property was added to make the routing more customisable, otherwise if routing is based on the assets then even a locked module will route to a linked page when a goto action is defined, which is not desired.

[chrismclarke]

nit(non-blocking): assuming the author doesn't provide these variables they will come through as undefined (and not null). Makes no real semantic difference, but might be easier to author the interface in the same way by marking optional properties with a ?, e.g.

interface IModuleListItemParams{

lockedImageAsset?: string
}

[chrismclarke]

Looks good to me, I agree with the minor comments @jfmcquade added but see no issue with merging here and adding as a follow-up (given all the code is nicely isolated to the plh components workspace)

[jfmcquade]

Thanks for making the updates @FaithDaka, I've added one minor commit in line with Chris's suggestion. I will merge this now.