-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
chore: fixing text variants, adding style prop and changing as to asChild #148
base: main
Are you sure you want to change the base?
Conversation
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
@@ -47,6 +47,7 @@ | |||
"test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch" | |||
}, | |||
"dependencies": { | |||
"@radix-ui/react-slot": "^1.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utilizes Radix Slot
, which provides asChild
functionality. See the technical spike documentation or the Radix docs for more details.
@@ -185,37 +185,58 @@ Use the boolean `ellipsis` prop to make the `Text` component have an ellipsis. | |||
|
|||
<Canvas of={TextStories.Ellipsis} /> | |||
|
|||
### As | |||
### AsChild |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating docs
'text-s-body-sm font-s-body-sm leading-s-body-sm tracking-s-body-sm md:text-l-body-sm', | ||
[TextVariant.BodyXs]: | ||
'text-s-body-xs font-s-body-xs leading-s-body-xs tracking-s-body-xs', | ||
'text-s-body-xs font-s-body-xs leading-s-body-xs tracking-s-body-xs md:text-l-body-xs', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes incorrect font sizes for BodySm and BodyXs
Before
before720.mov
After
after720.mov
@@ -159,19 +160,35 @@ export const Ellipsis: Story = { | |||
), | |||
}; | |||
|
|||
export const As: Story = { | |||
export const AsChild: Story = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating stories from As to AsChild
@@ -56,10 +56,10 @@ describe('Text Component', () => { | |||
expect(container.firstChild).toHaveClass('truncate'); | |||
}); | |||
|
|||
it('renders with custom HTML element when as prop is provided', () => { | |||
it('renders with asChild prop correctly', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating tests
@@ -117,28 +123,6 @@ export enum TextAlign { | |||
Justify = 'text-justify', | |||
} | |||
|
|||
export type ValidTag = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer needed as we can use the asChild prop
@SocketSecurity ignore-all Marked as acceptable risk as we are protected against supply chain attack with lavamoat |
8486a80
to
a39a11e
Compare
|
||
### Strong |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer supported we can add these styles to the root CSS stylesheet if we want all strong tags to be bold
<Text asChild className="block"> | ||
<span>Text rendered as span</span> | ||
</Text> | ||
<Text asChild className="block"> | ||
<button type="button" onClick={action('button-clicked')}> | ||
Text rendered as button | ||
</button> | ||
</Text> | ||
<Text asChild className="bg-default border border-default"> | ||
<input | ||
type="text" | ||
placeholder="Rendered as input" | ||
defaultValue="Text component as input" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a39a11e
to
f62ebe9
Compare
@@ -84,37 +83,6 @@ describe('Text Component', () => { | |||
}); | |||
}); | |||
|
|||
describe('HTML Elements', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer using ValidTag so we can remove this test and rely on Radix's Slot
Description
This PR introduces several important updates to the
Text
component in the design system react:style
prop to allow inline styles when needed for more flexible stylingBodySm
andBodyXs
with proper responsive font sizesasChild
prop to enable better component composition while maintaining styles as mentioned in the technical spikeThese changes improve the component's flexibility and maintainability while maintaining its core functionality.
Related issues
Fixes: #149
Manual testing steps
asChild
prop with different elements (span, button, input)Screenshots/Recordings
Before
Text component had limited styling options and no composition capabilities
before720.mov
After
asChild
after720.mov
Pre-merge author checklist
Pre-merge reviewer checklist