feat(@clayui/label): use a hybrid API appraoch to expose low-level components #3081
Conversation
Pull Request Test Coverage Report for Build 4678
💛 - Coveralls |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 082a842:
|
There was a problem hiding this comment.
hey @bryceosterhaus it looks good to me, I think it just got better and opens doors in the future for us to make improvements.
packages/clay-label/src/index.tsx
Outdated
| /** | ||
| * Path to the location of the spritemap resource used for Icon. | ||
| */ | ||
| spritemap?: string; | ||
| } | ||
|
|
||
| const ClayLabel = React.forwardRef<HTMLAnchorElement | HTMLSpanElement, IProps>( | ||
| const ClayLabelBase = React.forwardRef<HTMLSpanElement, IBaseProps>( |
There was a problem hiding this comment.
I think we went back to the name Base 😂, is a dead end!
There was a problem hiding this comment.
haha yeah I wasn't sure what else to call it. I'll update to be similar to our other components so that we avoid confusion.
There was a problem hiding this comment.
haha no problem. I think we can keep the Base.
packages/clay-label/src/index.tsx
Outdated
| {closeButtonProps && ( | ||
| <span className="label-item label-item-after"> | ||
| <ClayLabelItemAfter> | ||
| <button | ||
| {...closeButtonProps} | ||
| className="close" | ||
| className={classNames( | ||
| closeButtonProps.className, | ||
| 'close' | ||
| )} | ||
| type="button" | ||
| > | ||
| <Icon spritemap={spritemap} symbol="times" /> | ||
| <ClayIcon spritemap={spritemap} symbol="times" /> | ||
| </button> | ||
| </span> | ||
| </ClayLabelItemAfter> | ||
| )} |
There was a problem hiding this comment.
Maybe move this to ClayLabel below?
There was a problem hiding this comment.
I was debating back and forth whether this should be an easier API, or if we should require the end user to wire up the appropriate close button. I sort of like having it internal to ClayLabelBase since it doesn't prohibit or get in the way at all, but I also could see moving it to just the high-level. Are you thinking we would name it ClayLabelWithClose? If that's the case, we could do what we did modal(withTitle) and add withClose as a prop that defaults to true, and then the withClose logic would ultimately get extracted to a high-level component
There was a problem hiding this comment.
Yeah, actually, I wasn't thinking of creating a ClayLabelWithClose now, just thinking of making ClayLabelBase cleaner and currently moving it to ClayLabel only, we will still keep supporting it. I don't think we need withClose here yet. We can extract it in the future when we better consolidate the ClayLabel API.
|
@matuzalemsteles check out the latest commit for an example of adding the pattern that we did with Modal |
| </ClayLabelItemExpand> | ||
|
|
||
| {closeButtonProps && ( | ||
| <ClayLabelItemAfter> |
There was a problem hiding this comment.
It seems like this implementation doesn't consume ClayLabelItemBefore or am I wrong?
There was a problem hiding this comment.
Correct. Rather than add a new prop to the API, I went the route of breaking up Label into its low-level components so that users have more flexibility in how they want to use it. You can see the example of how we would add a before item at https://github.com/liferay/clay/pull/3081/files#diff-d25f4565a23c4c3cc5475dedd79d9ad1R62-R67
There was a problem hiding this comment.
Okay I see, good to know! Thanks
| const testRenderer = TestRenderer.create( | ||
| <ClayLabel.Base> | ||
| <ClayLabel.ItemExpand>{'Label'}</ClayLabel.ItemExpand> | ||
| <ClayLabel.ItemAfter>{'Content before'}</ClayLabel.ItemAfter> |
There was a problem hiding this comment.
before → after
Obligatory copy-pasta image:
bryceosterhaus
force-pushed
the
pr-3066
branch
3 times, most recently
from
964bccd to
70b33fe
Compare
|
hey @bryceosterhaus just to complement this change we can also add documents to clayui.com, perhaps to explain the addition of |
|
@matuzalemsteles good call. I will add that in this PR as well |
bryceosterhaus
changed the title
|
@matuzalemsteles update docs. let me know what you think |
| ## Overview | ||
|
|
||
| `ClayLabel` offers two different APIs for use by toggling the prop `withClose`. By default(`withClose={true}`), `ClayLabel` behaves like a high-level component. If you want to use the lower-level components and compose multiple parts to your label, all you need to do is set `withClose={false}`. | ||
|
|
||
| Check out [this storybook example](storybook.clayui.com/?path=/story/components-claylabel--w-content-before) for a demo. | ||
|
|
There was a problem hiding this comment.
@bryceosterhaus as it is a typical feature that we don't want to support for a long time, it might be worth adding a warning about it and that we will remove this double behavior in a major version later. What do you think?
There was a problem hiding this comment.
@matuzalemsteles I'm thinking its probably best to not mention that since they are essentially toggling the intended behavior of what this component will be. The next major version will likely behave the same even if withClose={false} is set.
When we do ever release 4.x, we can then note that with* props are moved to high-level components. But overall, I think it'd be unnecessary to mention at the moment since 4.x isn't really even on the radar yet. Once we get 4.x on the radar and have a better idea of what it would be, we can always add the note then.
continued conversation from #3066