Skip to content

Conversation

@tokimari
Copy link
Contributor

@tokimari tokimari commented Sep 4, 2025

SemiModal利用時に、SP幅の表示の際、コンテンツ領域が若干左寄りになっているのを修正しました。

variant before after
Popup image image
Sheet image image

対応方針

現状のPC幅の実装を見ると、スクロールバーの幅分右paddingを調整している↓ようだったので、SP幅もそれに倣ってpaddingを調整しました。

https://github.com/openameba/spindle/blob/main/packages/spindle-ui/src/Modal/SemiModal.css#L139

疑問1:

  • でもなんで左右均等の幅にしないんだろう??
  • Figmaではスクロールバーの領域は8pxじゃなくて12pxに見える 🤔
image

その他の方法

スクロールバーが不要なときにもスクロールバーの領域(8px)を取る実装になっている↓ので、

https://github.com/openameba/spindle/blob/main/packages/spindle-ui/src/Modal/SemiModal.css#L176

overflow: hidden auto; にすることで、必要な時だけスクロールバーの領域を取るのでも良さそう?
ただ、スクロールバーの有無でコンテンツの表示領域にブレが出るのが嫌で現状のpaddingの調整にしているのかなと想像し、選択しませんでした 🙏 もし意図わかる方がいれば 🙏

疑問2:SPのFigmaデザインではそもそもスクロール幅の領域を考慮されていなそう↓なので、こちらの選択肢の方が良いのかも?どうなんでしょう?

image

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2025

Visit the preview URL for this PR (updated for commit 28252d2):

https://ameba-spindle--pr1358-fix-semimodal-scroll-ts3fn8wz.web.app

(expires Sat, 04 Oct 2025 06:41:36 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: e7521619a2dd5c653490c8246e81ec2a5c8f1435

@reg-suit
Copy link

reg-suit bot commented Sep 4, 2025

reg-suit detected visual differences.

Check this report, and review them.

🔴 Changed 🔵 Passing
1 224
How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

@tokimari tokimari self-assigned this Sep 4, 2025
@tokimari
Copy link
Contributor Author

tokimari commented Sep 4, 2025

@yu-3in @yasuda-shin @herablog お手隙で〜〜〜 🙏

@yu-3in
Copy link
Contributor

yu-3in commented Sep 4, 2025

スクショ見る限り、今度はButtonが右に寄ってしまっている気がします

@tokimari
Copy link
Contributor Author

tokimari commented Sep 4, 2025

仕事が雑でした。出直します

@tokimari tokimari marked this pull request as draft December 10, 2025 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants