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

ボタンコンポーネントの実装 #21

Merged
merged 3 commits into from
Sep 21, 2022
Merged

Conversation

mehm8128
Copy link
Contributor

@mehm8128 mehm8128 commented Sep 17, 2022

close #17

Copy link
Member

@tesso57 tesso57 left a comment

Choose a reason for hiding this comment

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

LGTM !

PR出すときに、tesso, toshi, sappi_redあたりを Reviewersに指定していただけると助かります!

Copy link
Contributor

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

PRつくるときは close #17みたいにしておくとマージされたときにクローズされるのでそうしたほうがよいですね 😉

src/components/UI/BaseButton.vue Outdated Show resolved Hide resolved
src/components/UI/BaseButton.vue Outdated Show resolved Hide resolved
src/components/UI/BaseButton.vue Show resolved Hide resolved
src/styles/common.scss Outdated Show resolved Hide resolved
src/components/UI/BaseButton.vue Outdated Show resolved Hide resolved
justify-content: center;
gap: 4px;
padding: 4px 24px;
border-radius: 5px;
Copy link
Contributor

Choose a reason for hiding this comment

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

デザインは確認してないのですが、これは意図して4の倍数とか偶数ではない値になってますか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figmaで5pxになっていたのでそのまま使ってしまったのですが、アイコンありだと6pxになっていたので6pxにしようと思います

Copy link
Contributor

Choose a reason for hiding this comment

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

もし縦幅が変わることで変化するなら、px指定じゃなくて%指定のが正しそうです
(どっちの意図かはわからないです)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほどです🤔

Copy link
Contributor

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

一か所見落としていた箇所がありました 🙏

src/components/UI/BaseButton.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

:yosa::desu:

@tesso57
Copy link
Member

tesso57 commented Sep 21, 2022

↑ みどりさんから approve もらえたので、問題がなければ マージボタン押して大丈夫です!!
何か修正して、その部分が大きい場合は、Re-request review してください!

Copy link
Member

@toshi-pono toshi-pono left a comment

Choose a reason for hiding this comment

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

確認遅れてすみません、良さそうです:+1:

@mehm8128 mehm8128 merged commit 2da0e7a into main Sep 21, 2022
@mehm8128 mehm8128 deleted the feat/button-component branch September 21, 2022 12:04
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.

ボタンコンポーネントの実装
4 participants