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:フォロー/フォロワーを非公開としている場合、表示は「0」ではなく鍵アイコンを表示するように #10934

Closed
wants to merge 28 commits into from

Conversation

mappi-pr
Copy link
Contributor

@mappi-pr mappi-pr commented Jun 1, 2023

What

Feat #10887
今までフォロー/フォロワーを非公開にしていた場合、
クライアント上での表示が0/0となっていましたが
これを非公開/非公開としたいです。

また、設定が非公開となっている場合でもフォロー一覧、フォロワー一覧へのリンクが貼られてしまっているため
これについても非公開の場合にはリンクしないようにしています。

Why

フォロー/フォロワーを非公開にしたユーザが真に0/0なのか、非公開状態なのか見分けがつかないため
また、これにより一部のユーザが他ユーザのフォロー状況を正しく認知できていないため

Additional info (optional)

表示の範囲

  自分自身 フォローしているユーザ フォローしていないユーザ
公開 表示される 表示される 表示される
フォロワーのみ 表示される 表示される 「非公開」
非公開 表示される 「非公開」 「非公開」

コーディングについて

定数"isFfVisibility" を追加し無名関数で非公開状態のチェックをしてますが、
各画面で冗長的に処理が記載されております。
共通化すべきなど、Misskeyとしてのお作法がありましたらご教示いただけますでしょうか。

対応後イメージ

①ユーザページ
image

②フォロー一覧ページ
image

③タイムラインのポップアップ
image

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Jun 1, 2023
@github-actions github-actions bot requested review from acid-chicken and syuilo June 1, 2023 03:25
@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #10934 (c7c5121) into develop (84d3a06) will decrease coverage by 0.13%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop   #10934      +/-   ##
===========================================
- Coverage    77.33%   77.21%   -0.13%     
===========================================
  Files          907      737     -170     
  Lines        91480    70136   -21344     
  Branches      7546     7051     -495     
===========================================
- Hits         70748    54154   -16594     
+ Misses       20732    15982    -4750     

see 170 files with indirect coverage changes

@EbiseLutica
Copy link
Contributor

@mappi-pr PRありがとうございます🫶

定数"isFfVisibility" を追加し無名関数で非公開状態のチェックをしてますが、
各画面で冗長的に処理が記載されております。

各画面で共有するロジックは src/scripts に置いているので、そちらに共通化するとより良いかと思います!


「非公開」の表示ですが、漢字が並ぶのはデザイン上窮屈な印象を感じます - くらいで良いのかなと思っています

この辺は @syuilo の意見も気になるところです

私からは以上です 🙇🏻

@syuilo
Copy link
Member

syuilo commented Jun 4, 2023

ti-lock とか表示させるのでもいいかも

@mappi-pr
Copy link
Contributor Author

mappi-pr commented Jun 4, 2023

@EbiseLutica @syuilo
レビューいただきありがとうございます!

取り急ぎ表示の調整からさせていただきたく、
ti-lockで表示してみるとこんな感じになるのですが、いかがでしょうか・・・?
image

@syuilo
Copy link
Member

syuilo commented Jun 4, 2023

YOSASOU

@EbiseLutica
Copy link
Contributor

EbiseLutica commented Jun 4, 2023

ti-lock めっちゃいいですね!!

(タップしたらがちゃがちゃ動いて欲しいって思った)

@mappi-pr

This comment was marked as resolved.

@mappi-pr

This comment was marked as outdated.

@mappi-pr mappi-pr changed the title feat:フォロー/フォロワーを非公開としている場合、表示は「0」ではなく「非公開」とするように feat:フォロー/フォロワーを非公開としている場合、表示は「0」ではなく鍵アイコンを表示するように Jun 5, 2023
@mappi-pr
Copy link
Contributor Author

mappi-pr commented Jun 5, 2023

@EbiseLutica @syuilo
修正が完了いたしましたので、再度レビューお願いできますでしょうか。

レビュー後改定点

1. 非公開の場合の表示を鍵アイコンに変更
非公開の場合の表示を「非公開」の文字列から、鍵アイコンで表示するように変更いたしました。
これに伴い、PR/Chengelogのタイトルを変更いたしました。

2. 関数isFfVisibilityを共通関数化
非表示チェックを共通関数化し、/src/scripts 配下に新規ファイルとして切り出しました。
これに伴い、Null除けを考慮して一部処理順を変更しております。
(ログイン時・非ログイン時ともに、画面表示可能なことを確認済です)

3.マウスホバー時のアニメーションの追加
各画面にて、鍵アイコンがホバーされたときにロックされているかのようなガチャガチャとしたアニメーションを追加しました。
これらのアニメーションは、設定の「UIのアニメーションを減らす」で制御可能にしております。

mov.mp4

改定後イメージ

①ユーザページ
image

②フォロー一覧ページ
image

③タイムラインのポップアップ
image

Copy link
Contributor

@EbiseLutica EbiseLutica left a comment

Choose a reason for hiding this comment

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

私からは問題なしです 🙆🏻‍♂️

@@ -128,11 +142,43 @@ defineProps<{
.statusItemValue {
font-size: 1em;
color: var(--accent);

> i {
Copy link
Member

Choose a reason for hiding this comment

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

CSS Modulesではこういうセレクタは書きたくないかも

Comment on lines 227 to 232
> div {
> i {
display: block;
margin: 0 auto;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

同上

Copy link
Contributor Author

@mappi-pr mappi-pr Jun 10, 2023

Choose a reason for hiding this comment

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

@syuilo
ありがとうございます。
可読性と今後のメンテナンス性の観点でもう少し明示的かつ影響範囲が少ない形にしておきたいといったところ?と認識しました!

.keywiggleareaみたいに決め打ちのクラス名に適用される形で書き直してみる方針ではいかがでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CSSセレクタの書き方、修正してみましたのでイメージあっていたかどうかご確認いただけますでしょうか。

https://github.com/misskey-dev/misskey/pull/10934/files/e999fa0ed4fde43681f8af52130144dea23fe790..a787219cce24f7c8ce63fe56ce04bd5160f6916f

</template>
<template v-else>
<div :class="$style.statusItem">
<p :class="$style.statusItemLabel">{{ i18n.ts.following }}</p><span :class="$style.statusItemValue"><i class="ti ti-lock" :class="{[$style.animation]: animation}"></i></span>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p :class="$style.statusItemLabel">{{ i18n.ts.following }}</p><span :class="$style.statusItemValue"><i class="ti ti-lock" :class="{[$style.animation]: animation}"></i></span>
<p :class="$style.statusItemLabel">{{ i18n.ts.following }}</p><span :class="$style.statusItemValue"><i class="ti ti-lock" :class="{ [$style.animation]: animation }"></i></span>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ご指摘いただきありがとうございます!
各ファイル同様に6か所すべてに反映いたしました。

mappi-pr and others added 2 commits June 10, 2023 15:24
Co-authored-by: Acid Chicken (硫酸鶏) <root@acid-chicken.com>
@syuilo
Copy link
Member

syuilo commented Jul 8, 2023

ちょっと調整してからマージ

@syuilo syuilo closed this in 269cd56 Jul 18, 2023
@syuilo
Copy link
Member

syuilo commented Jul 18, 2023

単純に要素ごと消す実装にしてみました 🙏🏻

@mappi-pr
Copy link
Contributor Author

@syuilo
ありがとうございます💕

@mappi-pr mappi-pr deleted the feat_ffvisibulity branch July 18, 2023 10:58
slofp pushed a commit to Secineralyr/misskey.dream that referenced this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants