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: 画像縮小・非可逆圧縮の設定を分離 #179

Merged

Conversation

i544c-me
Copy link

What

cherry-picked from i544c-me#3
この PR では、4択となっていた画像圧縮の設定を2つのトグルに分け、ユーザーがより直感的に設定できるよう修正を行いました。

Before
スクリーンショット 2024-04-20 223655

After
スクリーンショット 2024-04-20 222914

Why

縮小の有無と非可逆圧縮の有無は分けて考えられますが、これを4択で提示してしまうとユーザーが混乱するのではないかと考えました。
2つのトグルならそれぞれ別に考えられますし、説明文も明確で分かりやすくなるかと思います。

Additional info (optional)

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

@Sayamame-beans
Copy link

Sayamame-beans commented Apr 20, 2024

ありがとうございます!

to anatawa12

  • 既存の設定が失われるので互換性のある形式にするかMigrationする

Copy link
Collaborator

@anatawa12 anatawa12 left a comment

Choose a reason for hiding this comment

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

ありがとうございます。

@i544c-me i544c-me requested a review from anatawa12 April 21, 2024 13:08
Copy link
Collaborator

@anatawa12 anatawa12 left a comment

Choose a reason for hiding this comment

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

ありがとうございます。デフォルト値に関する小さな修正をお願いします

@Sayamame-beans
Copy link

「オリジナル画像を保持しない場合に、Web公開用画像の圧縮形式を選択できます。」の話がなくなるので、何の場合に作用する設定なのか若干分かりづらくなる可能性が少しありそうですかね…? どうやって対応しようかな

Copy link
Collaborator

@anatawa12 anatawa12 left a comment

Choose a reason for hiding this comment

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

実装的にはOK

@i544c-me
Copy link
Author

「オリジナル画像を保持しない場合に、Web公開用画像の圧縮形式を選択できます。」の話がなくなるので、何の場合に作用する設定なのか若干分かりづらくなる可能性が少しありそうですかね…? どうやって対応しようかな

そうなんですよね...「オリジナル画像を保持する」にチェックを入れた時は、圧縮形式の設定2つを見えなくしてしまうか、トグルを disabled の状態にするかしたら、分かりやすいかなとは思いました。

@anatawa12
Copy link
Collaborator

そうなんですよね...「オリジナル画像を保持する」にチェックを入れた時は、圧縮形式の設定2つを見えなくしてしまうか、トグルを disabled の状態にするかしたら、分かりやすいかなとは思いました。

そうすると投稿画面でアップロード時にオリジナル画像を保持するをオフにした際の設定ができなくなるのでその実装では困ると思います

@i544c-me
Copy link
Author

なるほど、そういえば投稿画面でも保持の設定ありましたね...

では設定画面に説明を加える方向が良いですかね...?こんな感じはいかがでしょう?
image

Copy link
Collaborator

@anatawa12 anatawa12 left a comment

Choose a reason for hiding this comment

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

コードはよし。あとは翻訳さやまめさんお願いします

Copy link

@Sayamame-beans Sayamame-beans left a comment

Choose a reason for hiding this comment

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

これは原文からそうみたいなのですが、2560x2560"より小さくなるように"ではないような気がふとしました。
処理うろ覚えですが、2560x2560は大丈夫だった気がしていて…2560x2560"以下になるように"なのかな、と…
(そうだとして、本家側で表記修正してもらったタイミングで直すで良いかなぁという感じもあります)

@i544c-me i544c-me force-pushed the nirila/fix-drive-setting-page branch from 159359d to c627a21 Compare April 23, 2024 13:23
@i544c-me i544c-me force-pushed the nirila/fix-drive-setting-page branch from c627a21 to 432abd1 Compare April 23, 2024 13:32
@i544c-me
Copy link
Author

@Sayamame-beans ありがとうございます!修正取り込みました🙏

本家側で表記修正してもらったタイミングで直す

本家というのは misskey-dev/misskey ですかね?本家の方では 2560x2560 より小さくする説明自体無いような気がするのですが...
認識合っていれば、この PR で修正してしまって良いですかね...?

@Sayamame-beans
Copy link

本家の方では 2560x2560 より小さくする説明自体無いような気がするのですが...

確かにありませんでした(ごちゃまぜになってました、すみません)
修正助かります、お願いします🙏

@i544c-me
Copy link
Author

i544c-me commented Apr 23, 2024

修正しました!ご確認お願いします🙏

※2560x2560 "以下" になるのはここらへんがエビデンスになりそう...?
https://github.com/misskey-dev/browser-image-resizer/blob/2024.1.0/src/scaling_operations.ts

@i544c-me i544c-me requested a review from Sayamame-beans April 23, 2024 13:59
Copy link

@Sayamame-beans Sayamame-beans left a comment

Choose a reason for hiding this comment

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

見た感じ良さそうです!ありがとうございます!

@Sayamame-beans Sayamame-beans merged commit 13ff926 into niri-la:develop Apr 25, 2024
13 checks passed
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.

3 participants