feat(components): Improve list composability [JOB-107035]

[Aiden-Brine]

Motivations

The List component is rarely used because it can currently only be used to render list items in a very particular format. If consumers were able to specify how the items are rendered it could be used in many more places. This PR adds that flexibility.

Changes

The content prop in ListItemProps is now optional

Added

Before these changes ListItem was used to render the items passed into List. This PR adds a customRenderItem prop to List that will take precedence over ListItem. If no customRenderItem is provided the behaviour with using ListItem should be unchanged.

Changed

For existing List implementations there should be no change to functionality

Deprecated

Removed

Fixed

Security

Testing

Some before and after photos from testing things using the pre-release:

Selecting item type when adding a new item on the Schedule Alpha page

Before:

After:

Selecting a line item on the same page

Before:

After:

Promo stats here

Before:

After:

Christain Ellinger was kind enough to offer to test out the 5 instances in disputes as well, Also be sure to play around with my new stories in Storybook.

Changes can be tested via Pre-release

---

In Atlantis we use Github's built in pull request reviews.

[cloudflare-workers-and-pages[bot]]

Deploying atlantis with    Cloudflare Pages

Latest commit:	5c36dcd
Status:	✅  Deploy successful!
Preview URL:	https://7e2abcca.atlantis.pages.dev
Branch Preview URL:	https://job-107035-add-custom-render.atlantis.pages.dev

View logs

[ZakaryH]

I've been thinking about something we talked about last week, if the consumer is the one who wants to add a border bottom to their list items, how would they avoid the double border bottom?

      <List
        items={myAdvancedOptions}
        customRenderItem={item => (
          <div style={{ display: "flex", borderBottom: "1px solid #ddd" }}>
            <Text>{item.text}</Text>
            <span style={{ marginLeft: "auto" }}>{item.icon}</span>
          </div>
        )}
      />

the argument we give to the render function doesn't say if it's the first, last or middle item. without us providing it, they have to figure it out themselves.

the only way you can safely do it with a list whose order isn't static would be with CSS imo. you'd likely add a custom classname to the div which has the border bottom, and then say .myCustomItemClass:last-child { border: none }

that's not the worst honestly, but I am wondering about if there's a nice way for us to expose that internal state/knowledge the List component has to each of the items. what if the modification you want to do to the last item isn't CSS, and you can't just use last-child?

---

actually, is that true? or could you compute that yourself in your data? tracking which is the last item in your array, and safely assume that JS/React is going to render it in the order provided

I suppose that's possible. we'd just be asking multiple consumers to implement their own concept of isLastItem everywhere this is needed

I'm open to the argument this is over engineering, or solving a problem we haven't encounter yet. specifically the problem of a repeated pattern. seems like there would be some way for a consumer to solve this themselves. really a matter of if we like that solution or want to think about providing something from our own API.

[Aiden-Brine]

@ZakaryH you raise an interesting question. I previously was just thinking along the lines of CSS following the approach you mentioned "you'd likely add a custom classname to the div which has the border bottom, and then say .myCustomItemClass:last-child { border: none }". I hadn't thought about non-CSS use cases. A couple of options:

1) We change the typing for customRenderItem so that it takes in an optional index argument. Then List just always passes the index into customRenderItem when it calls it. If the consumer wrote their custom render function to use the index they will be provided it, if not, their render function will just ignore it

2) If we thought there are likely to be other pieces of info that the render function could need in the future we could say that the custom render function takes an options object like:

interface CustomRenderOptions<T> {
  item: string | number;
  index: number?;
  future options...
}

I think option 2 is definitely overkill right now, especially because of how simple List is I'm not sure what options we could possibly want to pass along with index in the future. I am on the fence about going option 1 or saying this is over engineering 🤔 Adding the optional index prop would be easy and improve the composability but it also adds one more piece of complexity and I'm not sure it is worth it. What do you think?

[ZakaryH]

@Aiden-Brine the more I think about this the more I'm talking myself out of it.

as a consumer you'll always have access to the data you're passing in. you'll have an opportunity to map over it and maybe add your own index or isLasItem, since we're not trying to do anything clever inside List. it simply maps over what was provided and renders it, meaning the order is entirely predictable/knowable.

with an extended type for items you can add whatever you want... so seems like you'd be able to solve a non-CSS last item problem yourself.

I think if we start seeing a repeated trend of people solving exclusively for the last item, then we should consider solving for it at a component level. so yeah, let's wait until it's an actual problem. we're not painting ourselves into a corner or making an unsolvable mess by waiting.

[Aiden-Brine]

@ZakaryH I'm on board with that. We could always add an optional prop to the render function if we wanted to in the future without much work or blowing things up.

[github-actions[bot]]

Published Pre-release for 1b9d5771d3badf51a7f24a5f3fb8c6b8ad9c7893 with versions:

  - @jobber/components@5.43.3-JOB-107035-1b9d577.3+1b9d5771

To install the new version(s) for Web run:

npm install @jobber/components@5.43.3-JOB-107035-1b9d577.3+1b9d5771

[Aiden-Brine]

@chris-at-jobber there are no design code changes for you to review but I tagged you in case you wanted to look at the two new Storybook stories I added based on your mockup

[chris-at-jobber]

@ZakaryH to the divider/avoiding a "double bottom border" question - is that aspect something List should have opinions about? ie

Either you want a divider, or you don't

• defaulting to having dividers
• If the divider is on a section header, use color-border--section

?

[ZakaryH]

to the divider/avoiding a "double bottom border" question - is that aspect something List should have opinions about? ie

Either you want a divider, or you don't

defaulting to having dividers If the divider is on a section header, use color-border--section ?

@chris-at-jobber do you mean the default version? that already has borders and manages them.

if you're referring to custom markup for list items, I'm hesitant to bake that in as behavior the List provides - unless we have strong opinions about the style/appearance of the border.

as for the section headers, in the case of custom markup we won't know much of anything about what they gave us. we can't really know if it's a header, a normal item or something else entirely. we're leaving that up to them to sort out.

[chris-at-jobber]

@ZakaryH sorry, I know that List already handles borders but we also have known, planned, instances that we won't want dividers on every element (Combobox, Menu) - just wondering how/if we will solve that!

[ZakaryH]

disregard, I made this in a silly way. better example exists

building out various custom lists and seeing how it feels **Sectioned List**  code to achieve this is... fine. I don't love the type narrowing needed to handle Icons names though. ``` const myItems: MyListItemProps[] = [ { id: 0, sectionTitle: "Country", }, { id: 1, name: "Canada", value: "CA", icon: "checkmark", }, { id: 2, name: "America", value: "USA", icon: "truck", }, { id: 3, name: "Mexico", value: "MX", icon: "quote", }, ]; return ( } /> ); } interface MyListItemProps extends BaseListItemProps { id: number; name?: string; value?: string; icon?: IconNames; sectionTitle?: string; } function ListItem(props: MyListItemProps) { if (props.sectionTitle) { return (

{props.sectionTitle}

); } if (props.icon) { return (

{props.name}

); } return <>; } ```

[ZakaryH]

explanation of move styles around to give customRender a blank slate

with the current List, the items are complex enough that we do the full-width + border bottom on it.

this is different from the simple, padded, rounded list, no-bordered style when the content is minimal

padding around it can be ignored. that's a concern of the element wrapping the List.

Focus/Hover State

currently this is all provided by the button's .hoverable class and .isActive

that's all well and good for the full width version, however if you want the rounded version with your custom markup we actually want that background to be on that element, not the one behind it otherwise you'd see the not-rounded edges peeking out behind the rounded ones if you tried to implement your own hover styles on your rounded element and the other one still exists behind it.

so instead, I've made the selector apply that to the first child of .isActive or .hoverable which is either the default container I've added, OR the custom markup provided.

that does mean that the custom markup provided needs a wrapping container that spans the full width but I think that's fine because it's going to yell at you about requiring a single parent container as the return from your customRender anyway.

button padding

we want the button. that's part of the value of List. it handles links and clickable items along with their hover/focus styles.

however we don't necessarily want the padding. the button wrapping our custom markup having padding means we can't use the full width causing issues with making your custom content line up nicely with the edges. with that, we've gotta remove it in the custom case and if you do want it, add it yourself.

Border Bottom on items

once again, this is part of the appearance for complex list items. we can't guarantee the custom content will be complex, so we should only apply it in the default case.

it does mean that someone providing custom markup will have to add their own border bottom, but I don't see any way around that. we just won't know what they're giving us. is it text and an icon, or a whole bunch of stuff?

Why now?

you could make the argument this is a requirement of Menu implementing List and we should do it at that point.

the reason I want to do it now is because it changes the canvas you are given for the custom markup, and I can see those limitations affecting how you implement custom markup. in the off chance someone uses this between now and then, it would become somewhat breaking change.

here's an example of building out that simple layout using these changes where I've made one row isActive and I'm hovering over it which gets a different color from the normal focus/hover

Example

 ``` const BasicTemplate: ComponentStory = args => (

); .... function RenderServiceProviderList({ listItem, }: { readonly listItem: SPListItemProps; }) { return (

{listItem.name}

); } ```

alternatively, if you do have busier content and the full-width + bordered style is desired with custom content

Example

 ``` const css = ` li:not(:last-child) .myContainer { border-bottom: 1px solid #e0e0e0; } `; const BasicTemplate: ComponentStory = args => ( ); function RenderServiceProviderList({ listItem, }: { readonly listItem: SPListItemProps; }) { return (

{listItem.name} Maintainer

); } ```

one thing to note about the above example is that you have to target the li:not(:last-child) .myContainer because those are the siblings. you can't do .myContainer:not(:last-child) because it's nested too far down from the siblings. that's how that selector works though, so maybe that's not even noteworthy at all. we can call it out in the docs regardless.

admittedly it is a little bit more for someone to implement the border bottom "logic" when all they wanted was custom content inside the full-width layout. unfortunately I don't see a better compromise that also enables the other padded, rounded layout 🤷

TL;DR -I moved some styles around so that we can use List + custom markup to make another type of list in Jobber that has a slightly different style

Pros simple lists that have rounded corners and no border bottom can be achieved as custom markup

Cons complex lists (which is what List is by default) with custom markup must re-implement

• border bottom css "logic"
• padding on their container element
• left/start alignment of text and flex

[ZakaryH]

this caught my eye while looking at border radius stuff. it's 100% existing and not something we've introduced.

since the section header doesn't have rounded corners, but the parent Card does - it cuts out the little intersection between them.

super minor and an interesting problem that would exist with any Card content that both has a background color AND has un-rounded borders 🤔

[ZakaryH]

@Aiden-Brine I thought about your question around if text-align: left should be in .defaultContent

the reason it was on .action is because button elements have a text-align: center from the user-agent stylesheet

while this is still technically a button, we are treating it more like a row of content where I'd argue having it be center aligned would be an outlier.

with that, I think we should put text-align: left back in .action so that you'll have the standard alignment by default. if you do want something centered, you can do that in your custom markup with an additional rule

---

looking at what's left in .defaultContainer we have: display: flex, box-sizing: border-box, flex:1

and in .defaultContent there's a lonely align-items: flex-start which only makes sense with flex. we are giving it flex, but in a kind of non obvious way via .defaultContainer > *

looking at our example usages it's also weird that we're giving a Flex component display: flex.Flex doesn't actually use flex haha

so we're trying to apply it though it's not doing anything since grid wins begging the question of why are we trying to make the children flex. the way we have this set up, people need to provide a wrapping container for their custom markup. I'd say it's fine to leave up to them to make it flex/grid/block whatever they want really.

border-box is somewhat useful but I don't see us needing to set that for everything.

TL;DR - I think all these can safely be moved into .defaultContent, allowing us to delete .defaultContainer, the React classname conditional, and omitDefaultStyles