-
Notifications
You must be signed in to change notification settings - Fork 152
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
ButtonLink to Tailwind #4044
ButtonLink to Tailwind #4044
Conversation
TODO:
|
Size Change: +183 B (0%) Total Size: 480 kB
ℹ️ View Unchanged
|
Deploying with Cloudflare Pages
|
9c99ba0
to
10f123e
Compare
3572349
to
5eaa12f
Compare
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.
Noticed some styles are off and bugs on the already implemented Button / ButtonPrimitive
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.
Can we call it styles instead? Calling it sizes is weirdly specific and non-accurate. WDYT?
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.
imo sizes
makes sense, since the intention of the file is to contain styles related to the Size
type. when sb will be looking for styles that are affected by the size
prop, this will be quite intuitive to look into. does this help with the context?
packages/orbit-components/src/tmp-tailwind/ButtonLink/README.md
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/tmp-tailwind/ButtonLink/index.js.flow
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/tmp-tailwind/ButtonLink/types.d.ts
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/tmp-tailwind/ButtonLink/index.tsx
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/tmp-tailwind/ButtonLink/index.tsx
Outdated
Show resolved
Hide resolved
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.
Noticed some styles are off and bugs on the already implemented Button / ButtonPrimitive
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.
Noticed some styles are off and spotted bugs on the already implemented Button / ButtonPrimitive
5eaa12f
to
5bffbce
Compare
packages/orbit-components/src/tmp-tailwind/ButtonLink/index.tsx
Outdated
Show resolved
Hide resolved
4ffe2e7
to
58229ac
Compare
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.
Issue about tokens and please chore the commits as talked IRL
"button-link-secondary-compact-foreground-hover": | ||
defaultTokens.colorTextButtonLinkSecondaryCompactHover, | ||
"button-link-secondary-compact-foreground-active": | ||
defaultTokens.colorTextButtonLinkSecondaryCompactActive, |
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.
We must not do this. These tokens are deprecated. We either create new tokens and this is automatically generated by the destructure above or we use the class for the expected color directly in the component.
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.
right 👍 I need to get in to the habit of checking for deprecation. these are actually the same as product-normal
colours so I used those
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.
done them commits
58229ac
to
e94d100
Compare
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.
e94d100
to
2194294
Compare
thank @DSil , fixed 👌 |
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.
Rebase and 🚀
6d350e6
to
dec0545
Compare
WEBC-1680
Storybook: https://orbit-mainframev-oreqizer-tw-button-link.surge.sh