refactor(Label): cleanup docs and simplify render #642
Conversation
dvdzkwsk
requested review from
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
| } | ||
|
|
||
| const imageElement = Image.create(image, { |
There was a problem hiding this comment.
Factories already handle falsy values in the first position, so we don't need to handle that ourselves.
src/components/Label/Label.tsx
Outdated
| <Layout | ||
| start={ | ||
| <> | ||
| {imagePosition === 'start' && imageElement} |
There was a problem hiding this comment.
Remove reassignment of local variables. We can simply render elements inline in their target position.
| icon?: ShorthandValue | ||
|
|
||
| /** An icon label can format an Icon to appear before or after the text in the label */ | ||
| /** A Label can position its Icon at the start or end of the layout. */ |
There was a problem hiding this comment.
Make terminology consistent. Allowed prop values are "start" and "end", so we should use the same wording in the description.
dvdzkwsk
force-pushed
the
refactor/label
branch
from
c2b3b04
to
d8434a9
Compare
dvdzkwsk
force-pushed
the
refactor/label
branch
from
d8434a9
to
8a51b36
Compare
kuzhelov
added
ready for merge
needs author changes
|
UTs need to pass before merging |
|
UTs are fixed now - there was a problem with filtering logic implemented as part of shorthand tests |
kuzhelov
removed
the
needs author changes
|
Thanks for the UT fix, @kuzhelov. |
…ardust-ui/react into refactor/label # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
I really like changes in this PR, superior work 👍
But, there is an issue that causes visual regressions. Layout will add a gap if start/end are present, after these changes they will be always present and it will produce invalid gaps 😭
We have already discussed this issue with @kuzhelov, we can make following changes:
const startImage = imagePosition === 'start' && imageElement
const starIcon = iconPosition === 'start' && iconElement
const hasStart = startImage || startIcon
<Layout start={hasStart && (
<>
{startImage}
{startIcon}
</>
)}It can be a solution, but I'm open for better ideas.
…ardust-ui/react into refactor/label # Conflicts: # CHANGELOG.md
…factor/label # Conflicts: # CHANGELOG.md
This PR cleans up some inconsistencies in the doc strings, and simplifies the render method for readability. There are no functionality changes; this is simply a reduction of source code.