Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: list item component to take children and add const assertion to colors #2337

Merged
merged 3 commits into from Jan 3, 2023

Conversation

kateinkim
Copy link
Contributor

Describe your PR

  • Adds const assertions to the color palette for better types.

CleanShot 2022-12-12 at 11 32 39@2x

  • When one of our clients was trying to add a nested view to the ListItem component, they had issues since the ListItem component was wrapping the children with the Text component with default styling which was messing up the alignment of the list. This PR gives the component more flexibility when it needs to take children without tx or text. Recreated the issue below and shows after the fix:
Case A Case B Fix
CleanShot 2022-12-12 at 10 57 22@2x CleanShot 2022-12-12 at 10 57 48@2x CleanShot 2022-12-12 at 10 54 31@2x

Copy link
Contributor

@frankcalise frankcalise left a comment

Choose a reason for hiding this comment

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

Nice work, left a comment for consideration!

{children}
</Text>
) : (
children
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, do we need any View styling around this or should we just let the user take care of that with what they're passing in?

$textStyles has some styling associated with it that we'd be missing here.

If we do add that wrapper though, then we'd need some sort of container override, so maybe better not to.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes more sense to just let the user style the children directly. The original issue the client had with this component was that $textStyles unintuitively styled the children being passed in so they were trying all sorts of different styles and getting unexpected results. I have a low gafo on this but I think adding additional styling would just result in the same issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kateinkim Sounds good to me!

Copy link
Member

Choose a reason for hiding this comment

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

There's a potential deprecation here with not allowing to pass other Text children into the (Text's are meant to be nestable here). I'm wondering if there's a better way to check what type children is without adding another prop... Thinking...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can just leave this PR for adding const assertion to the color palette and open an issue for the ListItem?

@frankcalise
Copy link
Contributor

@kateinkim do you want to remove the ListItem change for now and leave the color change? I'm going to publish an update with a few other changes so we can lump this one in with those

@kateinkim
Copy link
Contributor Author

@kateinkim do you want to remove the ListItem change for now and leave the color change? I'm going to publish an update with a few other changes so we can lump this one in with those

Yep! sounds good 👍🏼

@frankcalise frankcalise merged commit 7bdcec3 into master Jan 3, 2023
@frankcalise frankcalise deleted the fix/text-component-and-const-assertions branch January 3, 2023 19:19
infinitered-circleci pushed a commit that referenced this pull request Jan 3, 2023
## [8.4.5](v8.4.4...v8.4.5) (2023-01-03)

### Bug Fixes

* **cli:** fixed frontmatter clean up in generate cmd ([#2328](#2328) by [@joshuayoes](https://github.com/joshuayoes) and [@frankcalise](https://github.com/frankcalise)) ([f4788f9](f4788f9))
* list item component to take children and add const assertion to colors ([#2337](#2337) by [@kateinkim](https://github.com/kateinkim)) ([7bdcec3](7bdcec3))
* **boilerplate:** updated web drawer layout width ([#2342](#2342) by [@frankcalise](https://github.com/frankcalise)) ([58de713](58de713))
* **screens:** ErrorBoundary invalid should update ([#2323](#2323) by [@tschai-yim](https://github.com/tschai-yim)) ([3952b69](3952b69))
@infinitered-circleci
Copy link

🎉 This PR is included in version 8.4.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add const assertion to colour palette.
4 participants