Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

refactor(Label): cleanup docs and simplify render#642

Merged
layershifter merged 14 commits intomasterfrom
refactor/label
Jan 9, 2019
Merged

refactor(Label): cleanup docs and simplify render#642
layershifter merged 14 commits intomasterfrom
refactor/label

Conversation

@dvdzkwsk
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Factories already handle falsy values in the first position, so we don't need to handle that ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove reassignment of local variables. We can simply render elements inline in their target position.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make terminology consistent. Allowed prop values are "start" and "end", so we should use the same wording in the description.

Copy link
Contributor

@kuzhelov kuzhelov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

@kuzhelov kuzhelov added ready for merge needs author changes Author needs to implement changes before merge and removed ready for merge labels Dec 19, 2018
@kuzhelov
Copy link
Contributor

UTs need to pass before merging

@kuzhelov
Copy link
Contributor

UTs are fixed now - there was a problem with filtering logic implemented as part of shorthand tests

@kuzhelov kuzhelov removed the needs author changes Author needs to implement changes before merge label Dec 19, 2018
@dvdzkwsk
Copy link
Contributor Author

Thanks for the UT fix, @kuzhelov.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@layershifter layershifter merged commit 9c099a4 into master Jan 9, 2019
@layershifter layershifter deleted the refactor/label branch January 9, 2019 17:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants