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

Enforce Text suffix for Text-rendering components #3407

Merged
merged 5 commits into from
Apr 4, 2024
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 4, 2024

Follow-up to #3398.

This renames a bunch of components that always "promise" to render Text:

  • Description -> DescriptionText
  • InlineLink -> InlineLinkText
  • Label -> LabelText
  • Suffix -> SuffixText

It's not super "pretty" but it very clearly communicates the expectations that they render Text.

I've left H1..H7 and P "as is" because they're obviously text.

The last commit adds a lint rule check that enforces that such components always return Text.

Screenshot 2024-04-04 at 21 09 57

That's to prevent regressions.

This doesn't address buttons which I'll need to take a pass on separately.

Test Plan

Did this via IDE renames (TS language server). Walked around the app too.

Copy link

github-actions bot commented Apr 4, 2024

Old size New size Diff
6.34 MB 6.34 MB 0 B (0.00%)

Copy link
Member

@estrattonbailey estrattonbailey left a comment

Choose a reason for hiding this comment

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

Nice — honestly would be cool to pull this out in the future so 3rd parties can include in their RN projects, such a footgun and easy to miss

@haileyok
Copy link
Contributor

haileyok commented Apr 4, 2024

@estrattonbailey Not needed in this PR, but are we planning on using H1-7 at all? Can we rm if not? I think we decided not to but idr for sure.

@gaearon gaearon merged commit 3915bb4 into main Apr 4, 2024
6 checks passed
@gaearon gaearon deleted the text-suffix branch April 4, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants