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(spindle-ui): add link follow type in pagination #827

Merged
merged 4 commits into from
Oct 16, 2023

Conversation

Dai7Igarashi
Copy link
Contributor

概要

下記Design Docsに従い、Paginationのrel="nofollow"に関する修正を行いました。

close: #826

特記事項

指定必須のpropsを追加したので、BREAKING CHANGEです。

@Dai7Igarashi Dai7Igarashi self-assigned this Oct 12, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2023

Visit the preview URL for this PR (updated for commit 02dbb9e):

https://ameba-spindle--pr827-feat-add-linkfollowt-rznlzfuu.web.app

(expires Sun, 12 Nov 2023 07:39:18 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: e7521619a2dd5c653490c8246e81ec2a5c8f1435

@@ -30,15 +30,19 @@ import { Pagination } from './Pagination';
<Description>- `current`(必須): 現在のページ数を指定してください。</Description>
<Description>- `total`(必須): 総ページ数を指定してください。</Description>
<Description>
- `createUrl`(必須): 関数を渡すことでリンクのhrefとなる値を生成することが可能です。
- `linkFollowType`(必須): nofollowにするリンクのタイプを選択します。`all`,
Copy link
Member

Choose a reason for hiding this comment

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

imo: この説明だと nofollow にするタイプに対して all で全followになり少し混乱するので、ちと説明変えてもいいかもなぁ・・

@@ -30,6 +40,7 @@ export const PaginationItem: FC<Props> = React.memo(
current,
total,
});
const hasRelAttribute = getLinkRelAttribute({ linkFollowType, pageNumber });
Copy link
Member

Choose a reason for hiding this comment

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

ここまで来ないと pageNumber わからないのかぁ〜 (今更)

Copy link
Member

Choose a reason for hiding this comment

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

getLinkRelAttribute が bool 返ってくるのがちと違和感かも。名称をfollowのフラグ用途に絞るか、rel を扱うのであれば値返しちゃうのもあるかもです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nofollowundefinedを返しちゃおうと思います

@Dai7Igarashi Dai7Igarashi force-pushed the feat/add-linkFollowType-in-Pagination branch from 9cd9d15 to 4312df7 Compare October 13, 2023 02:45
@Dai7Igarashi Dai7Igarashi requested a review from herablog October 13, 2023 02:55
}
/>

## 全てのリンクをdofollowにする
Copy link
Member

Choose a reason for hiding this comment

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

ちと唐突なので、 linkFollowTypeの設定 あたりで見出しつけお願いしたいです〜。今の項目は一つ落としましょう。

Copy link
Contributor

@yasuda-shin yasuda-shin left a comment

Choose a reason for hiding this comment

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

少し気になった箇所コメントしました。
他は原さんご指摘いただいた感じで概ね良さそうな気がします。

case 'none':
return 'nofollow';
case 'firstPage':
if (pageNumber === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

case 'firstPage':
  return pageNumber === 1 ? undefined : 'nofollow';

☝️な感じで三項演算子使えばもうちょっとシュッとできそうな気がするのですがどでしょ。

@@ -30,15 +30,19 @@ import { Pagination } from './Pagination';
<Description>- `current`(必須): 現在のページ数を指定してください。</Description>
<Description>- `total`(必須): 総ページ数を指定してください。</Description>
<Description>
- `createUrl`(必須): 関数を渡すことでリンクのhrefとなる値を生成することが可能です。
- `linkFollowType`(必須): クロールさせたいリンクの種類を選択します。`all`,
Copy link
Member

Choose a reason for hiding this comment

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

下に詳細解説はあるのでここでは事実よりの説明でいいかもしれないです。なので説明文を以下の構成で変更お願いします!

「リンクのrelに設定する値をall, none, firstPage から選択できます。詳しくは「linkFollowTypeの設定」を参照してください」

ref: https://github.com/openameba/spindle/pull/827/files#r1357745061

@herablog
Copy link
Member

細かいですがコミットメッセージ以下な感じに変更したいです!

ae9aec5

BREAKING CHANGEのテキストは1文単体でわかる方が良さそうです、別枠で表示されることもあるので! ref: https://github.com/openameba/spindle/releases/tag/%40openameba%2Fspindle-ui%400.52.0

5d5e4cd

typeをtestに変更

4312df7

typeをdocsに変更

@Dai7Igarashi Dai7Igarashi force-pushed the feat/add-linkFollowType-in-Pagination branch from 4312df7 to 0c08260 Compare October 13, 2023 05:39
}
/>

<h2 id="link-follow-type-settings">linkFollowTypeの設定</h2>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[linkFollowTypeの設定](#link-follow-type-settings)からのリンク遷移のためにidを付与したかったのですが、mdxの記法でid付与が難しそうだったので、ここだけHTMLで直接書きました🙏

@Dai7Igarashi Dai7Igarashi force-pushed the feat/add-linkFollowType-in-Pagination branch from 0c08260 to 02dbb9e Compare October 13, 2023 07:32
Copy link
Member

@herablog herablog left a comment

Choose a reason for hiding this comment

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

エンジニアを褒めるネコ:リリースしました

@Dai7Igarashi Dai7Igarashi merged commit 4e60a9b into main Oct 16, 2023
@Dai7Igarashi Dai7Igarashi deleted the feat/add-linkFollowType-in-Pagination branch October 16, 2023 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Desing Docs: Paginationのnofollowの仕様変更
3 participants