Fix rendering choice-list child components

[vladucu]

What's here

This is a follow up on #318
Found out that with #318, we'll always render the wrapping Polaris-ChoiceList__ChoiceChildren div when there's a childComponent for a choice, which cause some extra spacing.

This:

• changes so that by default we render the childComponent only when the choice is selected
• adds an extra alwaysRenderChildComponent property, which when true will always render the childComponent

[andrewpye]

In the React implementation, the logic of whether the children should be rendered is completely independent of whether the choice is selected - that decision is left to the outside world. How do you feel about replacing this line with choice.shouldRenderChildComponent and let the outside world decide on this logic, the same way users of the React version can?

[vladucu]

I see what you mean...but how would that look in real world usage....can you add an example?

The React implementation just invokes a function and passes isSelected so the outside world can decide and return actual DOM....we could try to mimic this (just the deciding part) with an action for the choice, something like renderChildComponent....but although this matches a bit more what React implementation does....it's not really usage-friendly

[andrewpye]

I was thinking usage would look something like this, if we wanted to only render the child component when the item was selected:

<PolarisChoiceList
  ...
 @choices={{array
    (hash
      label="Option 1"
      value="one"
    )
    (hash
      label="Option 2"
      value="two"
      childComponent=(component "polaris-text-field" ...)
      shouldRenderChildComponent=(eq this.selected "two")
    )
  }}
  @selected={{this.selected}}
  ...
/>

This would, however, need the outside world to track whether each option was selected, which would likely be a bit clunky now I've looked into it.

Instead, how would you feel about a potential solution that's slightly hacky on the inside, but makes these problems go away for the outside world? 😅 What if we used the choice list's didRender/willRender hooks to auto-wrap/unwrap the children components based on whether anything's rendered or not? Means the outside world doesn't have to specify whether they want the child component to be rendered or not... 🤔

[vladucu]

Instead, how would you feel about a potential solution that's slightly hacky on the inside, but makes these problems go away for the outside world? 😅 What if we used the choice list's didRender/willRender hooks to auto-wrap/unwrap the children components based on whether anything's rendered or not? Means the outside world doesn't have to specify whether they want the child component to be rendered or not...

As you know from our current experience with auto-wrapping, that's pretty error-prone

As it's clear we can't match exactly the React implementation, I think we should aim for the simplest API - not saying what's in here now is it, but personally find it nicer than other suggestions.....open to others too...nothing else comes to my mind right now though

[andrewpye]

As you know from our current experience with auto-wrapping, that's pretty error-prone

Yes, although the thought above made me realise that we could look into using willRender to clean up auto-wrapped children. I've tried a quick experiment with it, but haven't been able to reproduce the issue we've seen before in testing 🤦‍♂

With that in mind, maybe we should go with what's here for now despite it being less than ideal, and update later if we're able to 🤷‍♂

[vladucu]

will add a TODO here and maybe we find something nicer soon

[andrewpye]

Looks like we've got some build issues on CI 😭

[vladucu]

Looks like we've got some build issues on CI

Seems like npm issues...Heroku builds are also failing