[AnnotatedSection] Update type definition for title prop to accept React.ReactNode

[francinen]

WHY are these changes introduced?

We came across a use case where we wanted to render another component, such as a Badge alongside the heading for an AnnotatedSection:

We considered passing a React component to the title prop that rendered both a Heading and a Badge component:

<Layout.AnnotatedSection
    title={
        <Stack>
            <Heading />
            <Badge />
        </Stack>
    }
>

While this renders successfuly in the browser, it raises a TypeScript error in the text editor because, at the moment, the title prop only supports strings.

After consulting @BPScott, we felt it seemed reasonable to change the type restriction for the title prop in this scenario: https://shopify.slack.com/archives/C8H54T86Q/p1557331031274400

WHAT is this pull request doing?

This PR updates the type definition for the title prop of the AnnotatedSection component to accept React.ReactNode instead of string.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Page} from '@shopify/polaris';

interface State {}

export default class Playground extends React.Component<never, State> {
  render() {
    return (
      <Page title="Playground">
        {/* Add the code you want to test here */}
      </Page>
    );
  }
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated the component's README.md with documentation changes
• Tophatted documentation changes in the style guide

[ghost]

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

[danrosenthal]

Looking at how AnnotatedSection handles a title, and it passes the prop down to its own Heading component. That component then wraps the title string in an h2. That wouldn't necessarily make for valid markup depending on what is passed here.

We should be conditionally wrapping it in the Heading based on the prop's type like we do in other components which have had this requirement relaxed. You'll also want to add tests for that condition

Example in Card

[francinen]

Thanks for pointing that out, @danrosenthal! Should I something to the README that recommends including a or

in the component being passed to the title prop?

[danrosenthal]

@francinen looks like this needs some formatting on the UNRELEASED doc

[BPScott]

Thanks for opening this up @francinen and sorry about being told one thing in slack and another in GitHub by different people.

---

We should be conditionally wrapping it in the Heading based on the prop's type like we do in other components which have had this requirement relaxed. You'll also want to add tests for that condition

I disagree with this behaviour. I think changing the wrapping component based up on if we pass a string or a ReactNode is very unintuitive and we should not do it.

I don't think consumers will realize that changing<AnnotatedSection title="foo" /> to <AnnotatedSection title={<>Foo <Badge>Hi</Badge></>} /> will delete the heading and that they'll need to add it themselves: (<AnnotatedSection title={<Heading>Foo <Badge>Hi</Badge></Heading>} /> to get the desired behaviour.

We want to discourage lots of content within the section header and I think making it so that your title is always wrapped in a <Heading> is a good way to discourage that - it makes it impossible to do complex layout style things.

---

It also looks like wrapping is currently the exception rather than the rule in the vast majority of cases:

Taking a look at existing components accept a prop that can be a ReactNode (and could be either a thus either a string or an element):

• <AccountConnection title> does not change its wrapping element

• <Modal title> does not change its wrapping element

• <Choice label helptext> do not change their wrapping elements

• <Connected left right> do not change their wrapping elements

• <EmptyState footerContent> does not change its wrapping element

• <FormLayout.Group helpText does not change its wrapping element

• <Labelled helpText> does not change its wrapping element

• <OptionList.Option label>does not change its wrapping element

• <RangeSlider helpText prefix suffix> do not change their wrapping elements

• <Select helpText> does not change its wrapping element

• <SettingAction action> does not change its wrapping element

• <TextField prefix suffix helpText connectedLeft connectedRight> do not change their wrapping elements

• <Card title> changes its wrapping element - removing the <Header>

• <Card.Section title> changes its wrapping element - removing the <Subheading>

• <Layout.AnnotatedSection description> changes its wrapping element - removing the <p>

• <Navigation.Item badge> changes its wrapping element - removing the <Badge>

• <Page title titleMetadata> takes a different approach - the title is always a string, but if you add a metadata then it gets injected next to the title. I dislike this and think we should let consumers deal with this layout instead of us trying to second-guess it.

In addition every instance of the children prop doesn't check if it was given a string, but I've omitted those from the list as thats not quite the same use-case and there's a lot of them.

Not wrapping is clearly a more prevalent option. It's only Card/Card.Section titles, Layout.AnnotatedSection descriptions and Navigation.Item badges that pull this trick.

[BPScott]

Also ping @ry5n and @yourpalsonja also as you both had opinions on this when I asked in Slack:

Ben:
FOLKS! Looks like we’ve found another case where people want us to widen some strictness in typing - they want to put in arbitary JSX instead of a string. Does anybody have a good reason why Layout.AnnotatedSection’s title should continue to be a string, instead of us widening the type to be React.Node so they can go put a badge next to their heading text.

Ryan:
I’m OK with this, but I’d want to expand design guidance to discourage lots of content in there. Previously in admin we had content and actions in there and they get lost visually. We’ve tried to get away from that.

Sonja:
If anyone is opening an issue for this, we should add a task in there about making sure we put some rationale in the styleguide docs.

[danrosenthal]

That's good feedback @BPScott! Thanks for pointing out that this is the exception rather than the rule. This feels like one of those best practices we should be documenting once we've reached consensus on it.

[BPScott]

@danrosenthal with all that in mind, do you have an updated opinion of what this PR should do?

Do you think we should keep the "only wrap in a Heading if it's a string" logic that @francinen added in 683c21a or should we revert that commit?

(For the record I'm firmly in the "avoid changing behaviour based on types, lets the revert that commit" camp)

[danrosenthal]

@danrosenthal with all that in mind, do you have an updated opinion of what this PR should do?

Do you think we should keep the "only wrap in a Heading if it's a string" logic that @francinen added in 683c21a or should we revert that commit?

(For the record I'm firmly in the "avoid changing behaviour based on types, lets the revert that commit" camp)

+1, this sounds like the right approach Ben. @francinen please let me know if you have any questions

[francinen]

Thanks for all the context, @BPScott and @danrosenthal! I reverted that last commit and fixed the formatting issues in the UNRELEASED.md.

[danrosenthal]

😅

Looks like a bad rebase happened here. Otherwise I'm good with this

[danrosenthal]

🚢 on 🍏

[BPScott]

Thanks!

[ghost]

🎉 Thanks for your contribution to Polaris React!