feat(Link)!: the "href" property is required #1270
Conversation
LakeVostok
requested review from
kseniya57,
amje,
ValeraS and
korvin89
as code owners
|
Preview is ready. |
LakeVostok
requested review from
resure,
IsaevAlexandr,
Raubzeug,
jhoncool and
goshander
as code owners
LakeVostok
force-pushed
the
chore/Link
branch
from
361654d to
8426404
Compare
LakeVostok
removed request for
resure,
goshander,
jhoncool,
IsaevAlexandr and
Raubzeug
| href?: undefined; | ||
| } & BaseBreadcrumbsItem; | ||
|
|
||
| export type BreadcrumbsItem = LinkBreadcrumbsItem | ButtonBreadcrumbsItem; |
There was a problem hiding this comment.
It was types product, became types sum. Its more powerful way for union declaration.
There was a problem hiding this comment.
I mean there is no difference, href is optional anyway. What's the point of this change?
There was a problem hiding this comment.
I mean there is no difference,
hrefis optional anyway. What's the point of this change?
Oh, I figure out, why it wasn't obvious to use this kind of approach. Lost it while rebasing, it should have been:
type BaseBreadcrumbsItem = {
text: string;
items?: BreadcrumbsItem[];
title?: string;
};
type LinkBreadcrumbsItem = {
href: string;
action?: (event: React.MouseEvent<HTMLElement, MouseEvent> | KeyboardEvent) => void;
} & BaseBreadcrumbsItem;
type ButtonBreadcrumbsItem = {
href?: undefined;
action: (event: React.MouseEvent<HTMLElement, MouseEvent> | KeyboardEvent) => void;
} & BaseBreadcrumbsItem;
So, we describe 2 categories of breadcrumb items: the one is like regular anchor(and should have the href prop), the two is like button, that should have some action(action prop).
So, if user want to create Breadcrumb, that is not like regular link, he will be forced(by typescript) to specify the action prop.
And if the developer(of uikit) needs to add a new behavior for only one of the types of breadcrumb item(e.g. the target attribute for anchor), then he modifies only LinkBreadcrumbsItem and any time in the code he will know, that he works with certain type of item(due to the href prop check).
| onClick: React.MouseEventHandler<HTMLElement>; | ||
| children: React.ReactNode; | ||
| }) { | ||
| return <button {...props} className={b('switcher', {more: true})} />; |
There was a problem hiding this comment.
button must have the type attribute with the "button" value
There was a problem hiding this comment.
And add please focus styling to the button
| | :--------- | :----------------------------------------- | :-------------------------------------------------------: | :--------: | | ||
| | view | Link appearance | `"normal" \| "primary" \| "secondary"` | `"normal"` | | ||
| | visitable | Display `:visitable` CSS state | `boolean \| undefined` | | ||
| | href | HTML `href` attribute | `string \| undefined` | |
There was a problem hiding this comment.
what exactly is broken and what tool to check it with? Github doesn't show any problems.
| @@ -2,7 +2,7 @@ import React from 'react'; | |||
|
|
|||
| import type {Meta, StoryFn} from '@storybook/react'; | |||
|
|
|||
| import {Link} from '../Link'; | |||
| import {Link, type LinkProps} from '../Link'; | |||
There was a problem hiding this comment.
We're not using this TS syntax
There was a problem hiding this comment.
Well, then it should be regulated by the linter. The language provides me with tools, and I use them. In addition, this constuction is used in other components(e.g. ActionTooltip).
I can set up an appropriate rule to ban inline type imports. But at first glance it looks strange. Why did you decide to not use it?
There was a problem hiding this comment.
I remember the main reason was that it requires to use TS 4.5+
LakeVostok
force-pushed
the
chore/Link
branch
from
8426404 to
df8cbe2
Compare
LakeVostok
force-pushed
the
chore/Link
branch
from
df8cbe2 to
5763523
Compare
amje
changed the title
UXRFC-1