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

fix(frontend): FormLinkをボタンとして使用した際にエラーが出る問題を修正 #13697

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

kakkokari-gtyih
Copy link
Contributor

@kakkokari-gtyih kakkokari-gtyih commented Apr 13, 2024

What

  • FormLinkをボタンとして使用した際(toを指定せずに使った場合)にエラーが出る問題を修正
  • FormLinkとMkFolderのデザインを統一
    • ヘッダーの文字の大きさが違ったので統一
    • FormLinkにもcaptionをつけられるように

Why

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

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Apr 13, 2024
@kakkokari-gtyih
Copy link
Contributor Author

Before

image

After

image

Copy link

codecov bot commented Apr 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.81%. Comparing base (f9aed8f) to head (541b696).

Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #13697       +/-   ##
============================================
+ Coverage    65.05%   77.81%   +12.75%     
============================================
  Files          985      184      -801     
  Lines       111876    25390    -86486     
  Branches      5723      487     -5236     
============================================
- Hits         72782    19756    -53026     
+ Misses       37660     5627    -32033     
+ Partials      1434        7     -1427     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@syuilo
Copy link
Member

syuilo commented Apr 14, 2024

  • その二つはたまたま似てるだけだから無理に統一させる必要はないかもしれない
  • FormLinkをボタンとして使うのを廃止した方が良い可能性がある

@syuilo
Copy link
Member

syuilo commented Apr 14, 2024

/preview

@kakkokari-gtyih
Copy link
Contributor Author

その二つはたまたま似てるだけだから無理に統一させる必要はないかもしれない

#13697 (comment) のように、並べるとちょっと違和感を感じるので統一するとすっきりしそう

@syuilo
Copy link
Member

syuilo commented Apr 14, 2024

たまたま似てるものを共通化するのは設計上悪手とされているから並ぶ時の見た目を揃えたいのであればそれ用のコンポーネント作るのが無難そうではある

@kakkokari-gtyih
Copy link
Contributor Author

たまたま似てるものを共通化するのは設計上悪手とされているから並ぶ時の見た目を揃えたいのであればそれ用のコンポーネント作るのが無難そうではある

作るか

@syuilo
Copy link
Member

syuilo commented Apr 14, 2024

これについては作るほどの必要性が微妙

@kakkokari-gtyih
Copy link
Contributor Author

一応作った

@syuilo
Copy link
Member

syuilo commented Apr 14, 2024

一応作った

この実装だとMkFolderとFormLinkが共通化されてしまっているわね(どちらもHeaderButtonを使わなければならないという暗黙の決まりができている)

@kakkokari-gtyih
Copy link
Contributor Author

うーむ

@kakkokari-gtyih
Copy link
Contributor Author

ただやはり並べてみたときの違和感はあるので何らかの形で統一しておいたほうがいい気はする

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.

None yet

2 participants