-
Notifications
You must be signed in to change notification settings - Fork 10
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
[WB-1874] Remove kind
prop now that secondary
kind is being deprecated
#2463
base: link-css-states
Are you sure you want to change the base?
Conversation
…condary` variant is now dropped.
🦋 Changeset detectedLatest commit: b739489 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: -184 B (-0.19%) Total Size: 97.3 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-uzrknytnsk.chromatic.com/ Chromatic results:
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (54f80ac) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2463" Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2463 |
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.
Looks good! Left some non-blocking questions!
@@ -0,0 +1,5 @@ | |||
--- | |||
"@khanacademy/wonder-blocks-link": major |
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.
Does this have an impact on Perseus?
@@ -40,7 +40,7 @@ export default { | |||
type StoryComponentType = StoryObj<typeof Link>; | |||
|
|||
/** | |||
* By default the link is a `primary` link. | |||
* By default the link uses a blue color. |
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.
Would it be helpful to reference the token name in docs instead of blue
since it might be a different color for other themes?
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.
hmm great question! how about adding a semantic meaning instead?
Something in the lines of: uses a color that communicates the presence and meaning of interaction
(extracted from the semantic colors docs).
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 sounds good!
if (kind === "secondary" && light) { | ||
throw new Error("Secondary Light links are not supported"); | ||
} | ||
|
||
if (visitable && kind !== "primary") { | ||
throw new Error("Only primary link is visitable"); | ||
} |
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.
🧹 Glad we can simplify things!
Summary:
Now that we are moving to Polaris, we’ve noticed that the Link.secondary variant
is not used very often (~0.2% of total Link usage). We are going to remove it to
simplify the styling of Links and consolidate on a more consistent pattern
across our sites.
This PR removes the
kind
prop from the Link component and updates thedocumentation to reflect this change.
Also removes the
kind="primary"
from the Link instance inBanner
component.Issue: WB-1874
Test plan:
Navigate to
/?path=/docs/packages-link--docs
and verify that the Linkcomponent does not have the
kind
prop anymore.Also navigate to
/?path=/story/packages-link-link-all-variants--default
andverify that the "kind" variants are not available anymore. Only "default" and
"inline".