Fix: Remove renderItem prop from font size picker

[jorgefilipecosta]

The FontSizePicker had a renderItem prop. But that prop made the default unused: https://github.com/ItsJonQ/g2/blob/3213c5991dd72c3ebe78eaa6d71e82beb3530ece/packages/components/src/font-size-control/font-size-control-select.js#L37-L40. That happens because when spreading even if the prop is undefined we are overwriting the default. The current font size picker on component does not offer a renderItem component so I guess we can continue to not offer it https://github.com/ItsJonQ/g2/blob/3213c5991dd72c3ebe78eaa6d71e82beb3530ece/packages/components/src/font-size-control/font-size-control-select.js#L55

[vercel[bot]]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/itsjonq/g2/fzssbcked
✅ Preview: https://g2-git-fork-jorgefilipecosta-fix-remove-rend-7c6d19.itsjonq.vercel.app

[ItsJonQ]

The current font size picker on component does not offer a renderItem component so I guess we can continue to not offer it

From a component library perspective, I think it's a good idea to expose renderItem. We may not need to use it within Gutenberg's font size picker (as the desired renderer is default in this FontSizePicker component). However, it being available enables 3rd party to fine-tune the UI if required. Otherwise, they would have to do it with less desirable methods, like forced CSS.

cc'ing @saramarcondes for thoughts <3

[jorgefilipecosta]

Hi @ItsJonQ,

on https://github.com/WordPress/gutenberg/pull/27594#discussion_r548022228 @youknowriad said:

Can we document the props of the component. Are these the exact same props as the existing component? What are the differences. For me personally, this is the most important thing when adding the G2 components: make sure the props make sense, that we don't have useless props and unnecessary public APIs. Also, ideally, the props don't have any DOM related thing to make them cross devices.

To me, the renderItem prop seems to be DOM related. People will use dom nodes in the renderItem property which will then not work on other devices. I think when people use a WordPress FontSizePicker they want the component as is, but if this proves not true and an issue appears where the prop is needed we can add it later. Adding it right now, when we still don't know if it will get considerable usage makes the need to maintain it forever even on future versions of the component.

[ItsJonQ]

To me, the renderItem prop seems to be DOM related.

@jorgefilipecosta For context, the renderItem API was inspired by implementations like FlatList from the world of React native: https://reactnative.dev/docs/flatlist#renderitem

I'm not sure how FontSizePicker would be used within the context of native development. As it stands, the current implementation isn't compatible with React Native (largely due to the Style System).

---

Adding it right now, when we still don't know if it will get considerable usage makes the need to maintain it forever even on future versions of the component

That's the decision with all APIs isn't it :).

From my side, the APIs I've designed into the G2 Components come from conventions/workflows inspired by other libraries within the UI space, expanding beyond web to include frameworks like Swift UI, React Native, and even Flutter.

(renderItem, and many more, would be considered one of those conventions)

I understand the hesitation with adding new APIs, which we may be able to control in the @wordpress/components side with some __unstable prefix. I don't think these concerns should come at a cost of dev. experience.

☝️ My sentiment there isn't targeted just to renderItem, but rather my experiences within the @wordpress/components ecosystem. I also don't mean to direct it at you - just expressing myself in the general sense.

[jorgefilipecosta]

That's the decision with all APIs isn't it :).

Exactly that is the reason why, we try to have the smallest public API possible and then expand it as soon as the need appears. Having a renderItem in the font size picker forces the component to be in a specific way. Imagine for some reason in the future we want to use a normal select for the font size picker. We would not be able to do that as we would not be back-compatible with the current plugins using renderItem to render things like div's and spans. Even if there is not a plugin doing that the fact that someone on private website may be doing that, makes the future changes hard.

I don't disagree with the pattern (having a renderItem) prop, I think for example on the SelectDropdown it may be useful. I'm just not sure if for the FontSizePicker it is something that we should add to the component.

[ItsJonQ]

@jorgefilipecosta Haiii

I just had a call with @saramarcondes to catch up on all the things. I was away for the later half of December - it's great to see progress on this.

I don't disagree with the pattern (having a renderItem) prop, I think for example on the SelectDropdown it may be useful. I'm just not sure if for the FontSizePicker it is something that we should add to the component.

After chatting with @saramarcondes and giving your feedback more thought... would it be correct to say that there's 2 "types" of UI components we'd offer in the components package:

• Common/Primitives
• More Gutenberg specific components

Common components like SelectDropdown (as you pointed out) are more open for general use. They can be used in a much wider context - for example, perhaps being used to render UI within WP Admin and beyond.

More Gutenberg specific components, like FontSizeControl, are slightly more limited.

If we're viewing things from that perspective, then I think it's acceptable to reduce the public APIs. (There's still a part of me that prefers to lean more towards optionality and flexibility)

Let me know what you think :)