Skip to content
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

feat(Link)!: the "href" property is required #1270

Merged
merged 2 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@
"build-storybook": "sb build -c .storybook -o storybook-static",
"lint:js": "eslint --ext .js,.jsx,.ts,.tsx .",
"lint:js:fix": "npm run lint:js -- --fix",
"lint:styles": "stylelint '{styles,src}/**/*.scss'",
"lint:styles:fix": "npm run lint:styles -- --fix",
"lint:prettier": "prettier --check '**/*.md'",
"lint:prettier:fix": "prettier --write '**/*.md'",
"lint:styles": "cross-env stylelint '{styles,src}/**/*.scss'",
"lint:styles:fix": "cross-env npm run lint:styles -- --fix",
"lint:prettier": "cross-env prettier --check '**/*.md'",
"lint:prettier:fix": "cross-env prettier --write '**/*.md'",
"lint": "run-p lint:*",
"typecheck": "tsc --noEmit",
"prepublishOnly": "npm run build",
Expand Down
13 changes: 12 additions & 1 deletion src/components/Breadcrumbs/Breadcrumbs.scss
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@use '../variables';
@use '../../../styles/mixins';

$block: '.#{variables.$ns}breadcrumbs';

Expand All @@ -12,7 +13,17 @@ $block: '.#{variables.$ns}breadcrumbs';
gap: 4px;
}

&__item {
&__switcher {
@include mixins.button-reset();
color: var(--g-color-text-secondary);

&:focus-visible {
outline: 2px solid var(--g-color-line-focus);
}
}

&__item,
&__switcher {
flex-shrink: 1;
display: inline-block;
overflow: hidden;
Expand Down
18 changes: 14 additions & 4 deletions src/components/Breadcrumbs/Breadcrumbs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,23 @@ import {BreadcrumbsSeparator} from './BreadcrumbsSeparator';

import './Breadcrumbs.scss';

export interface BreadcrumbsItem {
type BaseBreadcrumbsItem = {
text: string;
action: (event: React.MouseEvent<HTMLElement, MouseEvent> | KeyboardEvent) => void;
href?: 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;

export type BreadcrumbsItem = LinkBreadcrumbsItem | ButtonBreadcrumbsItem;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the same as before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was types product, became types sum. Its more powerful way for union declaration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean there is no difference, href is optional anyway. What's the point of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean there is no difference, href is 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).


export interface BreadcrumbsProps<T extends BreadcrumbsItem = BreadcrumbsItem> extends QAProps {
items: T[];
Expand Down
13 changes: 13 additions & 0 deletions src/components/Breadcrumbs/BreadcrumbsButton.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React from 'react';

import {block} from '../utils/cn';

const b = block('breadcrumbs');

export function BreadcrumbsButton(props: {
title: string;
onClick: React.MouseEventHandler<HTMLElement>;
children: React.ReactNode;
}) {
return <button {...props} type="button" className={b('switcher', {more: true})} />;
}
36 changes: 23 additions & 13 deletions src/components/Breadcrumbs/BreadcrumbsItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {Link} from '../Link';
import {block} from '../utils/cn';

import type {BreadcrumbsProps, BreadcrumbsItem as IBreadcrumbsItem} from './Breadcrumbs';
import {BreadcrumbsButton} from './BreadcrumbsButton';

interface Props<T extends IBreadcrumbsItem = IBreadcrumbsItem> {
data: T;
Expand All @@ -22,27 +23,36 @@ function Item<T extends IBreadcrumbsItem = IBreadcrumbsItem>({
isPrevCurrent,
renderItem,
}: Props<T>) {
const {text, title, href, action} = data;
const itemTitle = title || text;
const itemTitle = data.title || data.text;

const item = renderItem ? renderItem(data, isCurrent, isPrevCurrent) : data.text;

if (isPrevCurrent || !isCurrent) {
if (data.href !== undefined) {
return (
<Link
key={data.text}
view="secondary"
href={data.href}
title={itemTitle}
onClick={data.action}
className={b('item', {'prev-current': isPrevCurrent})}
>
{item}
</Link>
);
}

return (
<Link
key={text}
view="secondary"
href={href}
title={itemTitle}
onClick={action}
className={b('item', {'prev-current': isPrevCurrent})}
>
{renderItem ? renderItem(data, isCurrent, isPrevCurrent) : text}
</Link>
<BreadcrumbsButton key={data.text} title={itemTitle} onClick={data.action}>
{item}
</BreadcrumbsButton>
);
}

return (
<div title={itemTitle} className={b('item', {current: true})}>
{renderItem ? renderItem(data, isCurrent, isPrevCurrent) : text}
{item}
</div>
);
}
Expand Down
11 changes: 3 additions & 8 deletions src/components/Breadcrumbs/BreadcrumbsMore.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import React from 'react';

import {DropdownMenu} from '../DropdownMenu';
import {Link} from '../Link';
import {block} from '../utils/cn';

import type {BreadcrumbsProps} from './Breadcrumbs';
import {BreadcrumbsButton} from './BreadcrumbsButton';
import i18n from './i18n';

interface Props extends Pick<BreadcrumbsProps, 'popupPlacement' | 'popupStyle' | 'items'> {}
Expand All @@ -22,14 +22,9 @@ export function BreadcrumbsMore({popupStyle, popupPlacement, items}: Props) {
placement: popupPlacement,
}}
renderSwitcher={({onClick}) => (
<Link
view="secondary"
title={i18n('label_more')}
className={b('item', {more: true})}
onClick={onClick}
>
<BreadcrumbsButton title={i18n('label_more')} onClick={onClick}>
...
</Link>
</BreadcrumbsButton>
)}
/>
);
Expand Down
48 changes: 12 additions & 36 deletions src/components/Link/Link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,20 @@ export interface LinkProps extends DOMProps, QAProps {
view?: LinkView;
visitable?: boolean;
title?: string;
href?: string;
href: string;
target?: string;
rel?: string;
id?: string;
children?: React.ReactNode;
onClick?: React.MouseEventHandler<HTMLAnchorElement | HTMLSpanElement>;
onFocus?: React.FocusEventHandler<HTMLAnchorElement | HTMLSpanElement>;
onBlur?: React.FocusEventHandler<HTMLAnchorElement | HTMLSpanElement>;
extraProps?:
| React.AnchorHTMLAttributes<HTMLAnchorElement>
| React.HTMLAttributes<HTMLSpanElement>;
onClick?: React.MouseEventHandler<HTMLAnchorElement>;
onFocus?: React.FocusEventHandler<HTMLAnchorElement>;
onBlur?: React.FocusEventHandler<HTMLAnchorElement>;
extraProps?: React.AnchorHTMLAttributes<HTMLAnchorElement>;
}

const b = block('link');

export const Link = React.forwardRef<HTMLElement, LinkProps>(function Link(
export const Link = React.forwardRef<HTMLAnchorElement, LinkProps>(function Link(
{
view = 'normal',
visitable,
Expand Down Expand Up @@ -67,33 +65,11 @@ export const Link = React.forwardRef<HTMLElement, LinkProps>(function Link(
'data-qa': qa,
};

if (typeof href === 'string') {
const relProp = target === '_blank' && !rel ? 'noopener noreferrer' : rel;
const relProp = target === '_blank' && !rel ? 'noopener noreferrer' : rel;

return (
<a
{...(extraProps as React.AnchorHTMLAttributes<HTMLAnchorElement>)}
{...commonProps}
ref={ref as React.Ref<HTMLAnchorElement>}
href={href}
target={target}
rel={relProp}
>
{children}
</a>
);
} else {
return (
<span
{...(extraProps as React.HTMLAttributes<HTMLSpanElement>)}
{...commonProps}
ref={ref as React.Ref<HTMLSpanElement>}
// as this element has onClick handler
// eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex
tabIndex={0}
>
{children}
</span>
);
}
return (
<a {...extraProps} {...commonProps} ref={ref} href={href} target={target} rel={relProp}>
{children}
</a>
);
});
53 changes: 25 additions & 28 deletions src/components/Link/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,24 +110,21 @@ LANDING_BLOCK-->

## Href

The `href` property is optional. If it is skipped, the `Link` will behave like a `Button`.
The `href` property is required.

<!--LANDING_BLOCK
<ExampleBlock
code={`
<Link href="#">Link with href</Link>
<Link>Link without href</Link>
`}>
<UIKit.Link href="#">Link with href</UIKit.Link>
<UIKit.Link>Link without href</UIKit.Link>
</ExampleBlock>
LANDING_BLOCK-->

<!--GITHUB_BLOCK-->

```tsx
<Link href="#">Link with href</Link>
<Link>Link without href</Link>
```

<!--/GITHUB_BLOCK-->
Expand All @@ -140,17 +137,17 @@ A `Link` can be used both as an independent text element and as part of the text
<ExampleBlock
code={`
<Text>
<Link>what roles are active in the service</Link>
<Link href="#">what roles are active in the service</Link>
</Text>
<Text>
Currently, this role can only be assigned to a <Link>folder</Link> or <Link>cloud</Link>
Currently, this role can only be assigned to a <Link href="#">folder</Link> or <Link href="#">cloud</Link>
</Text>
`}>
<UIKit.Text>
<UIKit.Link>what roles are active in the service</UIKit.Link>
<UIKit.Link href="#">what roles are active in the service</UIKit.Link>
</UIKit.Text>
<UIKit.Text>
Currently, this role can only be assigned to a <UIKit.Link>folder</UIKit.Link> or <UIKit.Link>cloud</UIKit.Link>
Currently, this role can only be assigned to a <UIKit.Link href="#">folder</UIKit.Link> or <UIKit.Link href="#">cloud</UIKit.Link>
</UIKit.Text>
</ExampleBlock>
LANDING_BLOCK-->
Expand All @@ -159,32 +156,32 @@ LANDING_BLOCK-->

```tsx
<Text>
<Link>What roles are available in the service</Link>
<Link href="#">What roles are available in the service</Link>
</Text>
<Text>
Currently, this role can only be assigned to a <Link>folder</Link> or <Link>cloud</Link>
Currently, this role can only be assigned to a <Link href="#">folder</Link> or <Link href="#">cloud</Link>
</Text>
```

<!--/GITHUB_BLOCK-->

## Properties

| Name | Description | Type | Default |
| :--------- | :----------------------------------------- | :--------------------------------------------------------------------------: | :--------: |
| view | Link appearance | `"normal" \| "primary" \| "secondary"` | `"normal"` |
| visitable | Display `:visitable` CSS state | `boolean \| undefined` |
| href | HTML `href` attribute | `string \| undefined` |
| target | HTML `target` attribute | `string \| undefined` |
| rel | HTML `rel` attribute | `string \| undefined` |
| title | HTML `title` attribute | `string \| undefined` |
| children | Link content | `React.ReactNode` |
| extraProps | Any additional props | `Record \| undefined` |
| onClick | `click` event handler | `React.MouseEventHandler<HTMLAnchorElement \| HTMLSpanElement> \| undefined` |
| onFocus | `focus` event handler | `React.FocusEventHandler<HTMLAnchorElement \| HTMLSpanElement> \| undefined` |
| onBlur | `blur` event handler | `React.FocusEventHandler<HTMLAnchorElement \| HTMLSpanElement> \| undefined` |
| id | HTML `id` attribute | `string \| undefined` |
| style | HTML `style` attribute | `React.CSSProperties \| undefined` |
| className | HTML `class` attribute | `string \| undefined` |
| qa | HTML `data-qa` attribute, used for testing | `string \| undefined` |
| ref | React ref to Link DOM node | `React.ForwardedRef<HTMLElement> \| undefined` |
| Name | Description | Type | Default |
| :--------- | :----------------------------------------- | :-------------------------------------------------------: | :--------: |
| view | Link appearance | `"normal" \| "primary" \| "secondary"` | `"normal"` |
| visitable | Display `:visitable` CSS state | `boolean \| undefined` |
| href | HTML `href` attribute | `string \| undefined` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

table formatting broke

Copy link
Contributor Author

@LakeVostok LakeVostok Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what exactly is broken and what tool to check it with? Github doesn't show any problems.
йцук

| target | HTML `target` attribute | `string \| undefined` |
| rel | HTML `rel` attribute | `string \| undefined` |
| title | HTML `title` attribute | `string \| undefined` |
| children | Link content | `React.ReactNode` |
| extraProps | Any additional props | `Record \| undefined` |
| onClick | `click` event handler | `React.MouseEventHandler<HTMLAnchorElement> \| undefined` |
| onFocus | `focus` event handler | `React.FocusEventHandler<HTMLAnchorElement> \| undefined` |
| onBlur | `blur` event handler | `React.FocusEventHandler<HTMLAnchorElement> \| undefined` |
| id | HTML `id` attribute | `string \| undefined` |
| style | HTML `style` attribute | `React.CSSProperties \| undefined` |
| className | HTML `class` attribute | `string \| undefined` |
| qa | HTML `data-qa` attribute, used for testing | `string \| undefined` |
| ref | React ref to Link DOM node | `React.ForwardedRef<HTMLAnchorElement> \| undefined` |
3 changes: 2 additions & 1 deletion src/components/Link/__stories__/Link.new.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React from 'react';
import type {Meta, StoryFn} from '@storybook/react';

import {Link} from '../Link';
import type {LinkProps} from '../Link';

export default {
title: 'Components/Navigation/Link',
Expand Down Expand Up @@ -45,7 +46,7 @@ export default {
},
} as Meta;

export const Playground: StoryFn = (args) => {
export const Playground: StoryFn<LinkProps> = (args) => {
return <Link {...args} />;
};
Playground.storyName = 'Link';
15 changes: 3 additions & 12 deletions src/components/Link/__stories__/LinkShowcase.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import React from 'react';

import {Showcase} from '../../../demo/Showcase';
import {ShowcaseGrid} from '../../../demo/ShowcaseGrid';
import {ShowcaseGrid} from '../../../demo/ShowcaseGrid/ShowcaseGrid';
import {Link} from '../Link';

export function LinkShowcase() {
return (
<Showcase title="Link">
<ShowcaseGrid
rowKey="href"
rowKey="target"
component={Link}
propsCombinations={{
view: [
Expand All @@ -25,16 +25,6 @@ export function LinkShowcase() {
value: 'secondary',
},
],
href: [
{
name: 'With href',
value: '#',
},
{
name: 'Without href',
value: undefined,
},
],
visitable: [
{
name: 'Not visitable',
Expand All @@ -58,6 +48,7 @@ export function LinkShowcase() {
}}
staticProps={{
children: 'Link',
href: '#',
}}
/>
</Showcase>
Expand Down
Loading
Loading