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

feat(Text, Flex, Box): typed html attributes #1583

Merged

Conversation

IsaevAlexandr
Copy link
Contributor

@IsaevAlexandr IsaevAlexandr commented May 15, 2024

  • fully types DOM attributed depends on as props for Text, Box and Flex components with backward compatibility;
  • added tests for Text component;

related to #1581

@IsaevAlexandr IsaevAlexandr requested a review from amje May 15, 2024 13:38
@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui-bot
Copy link
Contributor

Playwright Test Component is ready.

@IsaevAlexandr IsaevAlexandr force-pushed the feat/add-more-attrs-to-text branch 3 times, most recently from cc15eba to 3abfe9e Compare May 15, 2024 16:42
@IsaevAlexandr IsaevAlexandr requested a review from korvin89 May 15, 2024 16:44
@chelentos
Copy link
Contributor

chelentos commented May 16, 2024

I doubt the correctness of using of Box for Text and Flex. Semantically, these components are needed for different tasks. For example, does the Text component need props for its width/height and spacing?

@IsaevAlexandr
Copy link
Contributor Author

I doubt the correctness of using of Box for Text. Semantically, these components are needed for different tasks. For example, does the Text component need props for its width/height and spacing?

these are reasonable remarks. The use of the Box component in this example is necessary in order to work correctly with basic props such as qa, styles, className and ref and other. Like base logic fore thouse examples. Out of the box you get the ability to use Text component something like this:

<>
  <Text spacing={{mr: 2}}>Some text 1</Text>
  <Text>Some text 1</Text>
<>

@chelentos
Copy link
Contributor

Out of the box you get the ability to use Text component something like this:

<>
  <Text spacing={{mr: 2}}>Some text 1</Text>
  <Text>Some text 1</Text>
<>

I think the question then might be why are we adding this behavior for Text only?

@IsaevAlexandr
Copy link
Contributor Author

IsaevAlexandr commented May 16, 2024

Out of the box you get the ability to use Text component something like this:

<>
  <Text spacing={{mr: 2}}>Some text 1</Text>
  <Text>Some text 1</Text>
<>

I think the question then might be why are we adding this behavior for Text only?

Text is one more in a row. Flex, Card and, i hope, it will be more components soon what extends from Box

@IsaevAlexandr IsaevAlexandr force-pushed the feat/add-more-attrs-to-text branch from 3abfe9e to 0909c60 Compare May 16, 2024 09:27
@IsaevAlexandr IsaevAlexandr changed the title feat(Text): allow pass thml attributes durind Box inheritance feat(Text, Flex, Box): typed html attributes May 21, 2024
@IsaevAlexandr IsaevAlexandr force-pushed the feat/add-more-attrs-to-text branch from 0909c60 to 90b9863 Compare May 21, 2024 14:01
@IsaevAlexandr IsaevAlexandr merged commit 3345489 into gravity-ui:main May 21, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants