7683 Make divider explicit and configurable

[allthesignals]

🎫 Ticket

Link to the relevant ticket.

🛠 Summary of changes

Previously, troubleshooting options relied on implicit logic for dealing with the red divider, specifically with multiple invocations of the component. Now the divider is explicitly added, default to appearing. IPP view uses this to show the divider in the middle.

📜 Testing Plan

• Manual visual check locally
• Visual check/diff in other areas of the app
• CI checks pass

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before:

After:

[allthesignals]

At first I wasn't clear why there was a separate options prop, but I see now that it's mapped into some parts of the component... and it refers to the "options" in the sense of the domain, not component-specific "options" you'd think of in a reusable library.

So, I put the divider option as a top-level prop.

[aduth]

At first I wasn't clear why there was a separate options prop, but I see now that it's mapped into some parts of the component... and it refers to the "options" in the sense of the domain, not component-specific "options" you'd think of in a reusable library.

That confusion hadn't occurred to me before, but I can see it now that you mention it. Would you have any recommendations on how to make it clearer? Maybe calling it links ?

[allthesignals]

I made the divider more explicit. Before, we had the CSS controlling what to do when there were multiple instances of the same troubleshooting-options class.

[aduth]

I made the divider more explicit. Before, we had the CSS controlling what to do when there were multiple instances of the same troubleshooting-options class.

👍

A small implementation suggestion could be to align it with how we apply conditional classes in other components, as a filtered and joined array:

const classes = [
  'troubleshooting-options',
  divider && 'troubleshooting-options--divider',
].filter(Boolean).join(' ');

return (
  <section className={classes}>

(In other projects I've used a library like classnames or clsx to do this, this is just a native JS equivalent)

[aduth]

Also, this implementation would output a literal string "false" in the class name if rendered without a divider, since the left-hand value would be used in a falsey &&, and false becomes 'false' when concatenated into a string (false.toString()).

`troubleshooting-options ${false && 'troubleshooting-options--bar'}`
// 'troubleshooting-options false'

[allthesignals]

Oof! Thank you! This is a pattern I'd been using in other projects.

[allthesignals]

Not sure I think "bar" is the right word for this. Probably should just call it "divider" like I call it in the prop.

Could also be longer and more semantic horizontal-rule

[aduth]

Not sure I think "bar" is the right word for this. Probably should just call it "divider" like I call it in the prop.

Yeah, I think it'd be good to match the prop name. No strong preference on name, but "divider" seems appropriate.

[aduth]

Couple suggestions, but overall LGTM 👍

[aduth]

Not sure I think "bar" is the right word for this. Probably should just call it "divider" like I call it in the prop.

Yeah, I think it'd be good to match the prop name. No strong preference on name, but "divider" seems appropriate.

[aduth]

I made the divider more explicit. Before, we had the CSS controlling what to do when there were multiple instances of the same troubleshooting-options class.

👍

A small implementation suggestion could be to align it with how we apply conditional classes in other components, as a filtered and joined array:

const classes = [
  'troubleshooting-options',
  divider && 'troubleshooting-options--divider',
].filter(Boolean).join(' ');

return (
  <section className={classes}>

(In other projects I've used a library like classnames or clsx to do this, this is just a native JS equivalent)

[aduth]

Can we add a test case(s) that the expected class is added / not added depending on this prop?

[aduth]

I don't think this assertion covers the behavior we're expecting from the divider prop.

For example:

  it('renders with expected classes', () => {
    const { container } = render(<TroubleshootingOptions {...DEFAULT_PROPS} />);

    const element = container.firstElementChild!;

    expect(element.classList.contains('troubleshooting-options')).to.be.true();
    expect(element.classList.contains('troubleshooting-options--divider')).to.be.true();
  });

  context('with divider disabled', () => {
    it('renders with expected classes', () => {
      const { container } = render(<TroubleshootingOptions {...DEFAULT_PROPS} divider={false} />);

      const element = container.firstElementChild!;

      expect(element.classList.contains('troubleshooting-options')).to.be.true();
      expect(element.classList.contains('troubleshooting-options--divider')).to.be.false();
    });
  });

[allthesignals]

Ah, yes. My bad. Thank you.

Why assert troubleshooting-options presence, too?

[aduth]

Why assert troubleshooting-options presence, too?

For me, a combination of:

1. Making sure we're adding the class, since it's not already covered by another spec, and it's necessary for it to be added for the element to be styled correctly
2. As a confidence check that we're asserting against the expected element, since at least in the second test case expect(element.classList.contains('troubleshooting-options--divider')).to.be.false() could pass if element were something completely different than what we expect (e.g. document.body). The fact we have to dig through the DOM to get to the element with firstElementChild makes it more of a concern for me.