Passing React Elements vs. string props

[yurm04]

We often get requests on our components to allow React Elements to be passed to props rather than a specific prop type e.g. string

Most of our components today have strong opinions about what props they expect. While we could make changes to component APIs ad hoc as requested, this is probably better solved by coming up with an opinion on our component APIs and composability.

Some recent examples of requests and components:

EmptyState and the passed image url prop

MediaCard component and the title prop

Tag

Button

Page component and subtitle prop

[mbaumbach]

From a consumer perspective on the MediaCard, we sometimes want to put other small decorations on the title or description. These decorations might be like a Badge or perhaps make part of the content be bold using TextStyle or adding a "Learn more" link to the end of the description. At the moment, the title currently supports ReactNode but the description is currently string, though we tend to add those kinds of decorations to the description rather than title.

I certainly appreciate the perspective of keeping component props very specific such that the style is controlled and consistent. I'm generally in favor of more controlled as I think it makes the API surface of a component simpler to grok. It also lets the component props describe data/content and defer visual decisions to the Polaris/Design teams. I think that's better for future proofing Polaris, in general.

In the end, I'm supportive of an opinionated framework/design and if a consumer wants more customization it's ok to require a custom component to be built by the end users. That's typically what we've done whenever we need to step outside of the constraints of a particular component.

Having discussions like this is super helpful though! I love to see partners and Shopify devs collaborate on ideas for enhancing components.

[jakemoves]

I recently went through this journey. UX specified a clickable table cell with a two- or three-line label, with hard line breaks.

For semantic and accessibility reasons, I wanted to use a <button> (or, less desirable but still okay, an <a>) element inside the cell for this feature.

At first (being new to Shopify and Polaris... and React) I tried a Button, but they only accept string content. I considered making a proposal around this to the Polaris team, but I noticed the HTML spec suggests <button>s should only contain phrasing content, so I felt that I couldn't make a strong case for changing the Button API.

I rolled my own solution based on <button> but that got fussy when I wanted to choreograph some focus handling.

I then switched to a Link, which allows node content as children. This made me unhappy because I'd have to roll my own routing to create a valid url prop for the Link. However, I noticed that when a url tag is not provided, the Link renders with a <button> instead of an <a>. So now I have a Polaris-wrapped <button>, which is what I wanted anyways.

Like I said, it was a journey.

In my case, being able to pass phrasing content as a child into Button would have solved this with the very semantic and simple solution of the <br> tag (while still matching the HTML spec for <button> and not throwing open the doors to any and all types of children). Also, the documentation on Link could have mentioned the behaviour I ended up relying on.

[tatianau]

Glad I came across this discussion as I think about this a lot.

In my case, I’ve run into the need of using ReactNode props to resolve accessibility concerns - returning the focus to the element activator. When a <Page />'s secondary action opens a modal - changing the page action props to a custom node allowed us to pass a custom action with a ref to be able to return the focus to the modal activator after it’s closed.

The problem with this is that it only works when a page has one secondary action. After having to add another action, it’s become an issue for us as consumers as we were on the crossroads of whether to build all the features we were getting from <Page /> actions subcomponents for free before, e.g. collapsing the actions on smaller screens with less real estate, dropdown focus management, and so on. And doing so, regardless of the fact that it was not feasible for our project timelines, would potentially open the door for other deviations from the design system. So we ended up pivoting to using the default secondaryActions prop to leverage all of that and leave focus issue resolution for later because it’s prevalent across all other admin sections' indexes for the same reasons.

I now regret changing the <Page /> API to ReactNode as it only resolves the focus management issue for cases when there is only one or sometimes two if very tiny secondary actions on the page, which is almost never the case. Although B2B folks were able to use it to solve the same issue in the Price Lists section as they only had one secondary page action at that point. What I'm concerned about is that changing the component API to ReactNode to only accommodate what turned out to be a very specific use case also opened a black hole in the design system. And I know it's hard to find a balance between allowing for customizability while being opinionated about how it's being used by consumers for the system health, and I wonder if there are other ways to solve accessibility issues like that across the system, so thank you for opening this discussion. 🙏

[pbojinov]

We came across this issue issue in the Notebooks app when we wanted to have an editable Notebook title but weren't able to do so using <Page/>, so we ended up having to create our own Page component to our best ability. I would love to be able to use Polaris Page and benefit from all the amazing breakpoint responsiveness and accessibility that we're potentially missing by using our own implementation.

I personally like the flexibility of being able to pass in a string or formatted object when there aren't custom component needs but also having the flexibility for a React.ReactNode when needed. E.g. The flexibility that primaryAction, secondaryActions, and additionalMetadata have.

The flip side of this flexibility is that users might do things that we don't anticipate or want to support in the design system.

[devonpmack]

May not be ideal but by keeping it string people can still get around it with as unknown as string

It's still strongly suggested to use string but if a ReactNode is needed then it can be bypassed without changing the Polaris props when direly needed.

[jakemoves]

I'm new-ish to Typescript, that's super interesting! I would love to hear more about this — opinions, examples in the codebase... how many years you should work here before resorting to that... 😂

[claudioc]

@jakemoves the thing is, when you "force" TS to pass the Button a react node (and not just a string), it works. I didn't expect it to and I did it unwillingly, because I needed an Icon in there. Once you realize that, you may consider to keep the "force" in place or not - but of course you should not (you can use an UnstyledButton in that case).

[jakemoves]

Hmm. I'm going to try both, but since I actually want most of the styling from Button, and I'm feeding it a simple phrasing tag rather than a complex React node, I lean towards the type dodge?

[devonpmack]

Must be at least L6 to use this simple trick 😂

[pbojinov]

I didn’t realize it was possible to pass in a React component with a type
override. I tried it and it worked. Thank you for sharing that.

As for whether we should actually do that, probably not, that’s another
question.