-
Notifications
You must be signed in to change notification settings - Fork 23
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
Link Component #5502
base: main
Are you sure you want to change the base?
Link Component #5502
Conversation
🦋 Changeset detectedLatest commit: 0e52fc5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
return ( | ||
<FrontendServices {...config}> | ||
{/* application code */} | ||
<RacRouterProvider navigate={(href, opts) => router.push(href, undefined, opts)}> |
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 make this provider part of next-services so it works for the consuming teams out of the box and without extra config?
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.
Hey @jakubriedl did you mean before this release goes out?
I certainly think that makes sense as a future / subsequent goal, but don't think it should block this specific release since the component is agnostic to the routing solution and standard hrefs will work out of the box.
Potentially a good opportunity for @Zystix once this is done (noting we only have him till the end of Feb), but will also raise with the team to see when we can prioritize the work
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.
yeah totally agree this shouldn't be blocker. But I'd add it there at the same time as it's just small change and write the documentation in the expects it to be there. It'll save you a lot of explaining to teams what they have to do, just say in the docs "have up-to-date next-services" and you can skip this whole section. Easier for you, and for the consumer as well
packages/components/src/Link/_docs/Link--api-usage-guidelines.mdx
Outdated
Show resolved
Hide resolved
<Controls | ||
of={Link.Playground} | ||
include={['href', 'variant', 'size', 'isDisabled', 'icon', 'iconPosition']} | ||
className="mb-64" | ||
/> |
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.
Since we're exposing the storybook controls for something like icon, which needs a JSX element we're going to need to create a mapping in the argsTypes to provide a could of options for anyone landing on this page :)
In our Link.docs.stories
.tsx what we'll want to do is something like this:
const meta = {
// ...other meta properties
argTypes: {
// creates a mapping for the icon property
icon: {
// provide 3 options they can select
options: ['delete', 'arrow', 'add'],
mapping: {
// maps each of these keys to the options (needs to exactly match)
delete: <Icon isPresentational name="delete" />,
arrow: <Icon isPresentational name="arrow_forward" />,
add: <Icon isPresentational name="add" />,
},
},
},
isUnderlined: true, | ||
isReversed: false, | ||
size: 'body', | ||
}, |
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 is where we want to add those mappings for icon, otherwise folks will get this error if they try to update the icon property on this page 😉
Why
Creating a new Link component.
https://cultureamp.atlassian.net/browse/KZN-2593
What
Adds the Link component, storybook tests, sticker sheets, API spec and usage guidelines.