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: add tooltip component #738

Merged
merged 12 commits into from
May 1, 2020
Merged

Conversation

AtsushiM
Copy link
Member

Tooltip Componentを追加します。

@AtsushiM AtsushiM requested a review from a team as a code owner April 14, 2020 09:13
@AtsushiM AtsushiM requested review from Tokky0425 and removed request for a team April 14, 2020 09:13
@netlify
Copy link

netlify bot commented Apr 14, 2020

Deploy preview for smarthr-ui ready!

Built with commit ee74e74

https://deploy-preview-738--smarthr-ui.netlify.app

Copy link
Member

@Tokky0425 Tokky0425 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/Tooltip/Tooltip.stories.tsx Outdated Show resolved Hide resolved
src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved
src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved
src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved
src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved
src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved
src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved
src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved
Copy link
Member

@nabeliwo nabeliwo left a comment

Choose a reason for hiding this comment

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

レビュワーではないのですが2点気になったのでコメントします!

  1. テスト落ちているのでマージ前に対応されるとは思いますが、ユーザから使えるようにルートの index.ts で Tooltip component の export をお願いします 🙏

  2. Tooltip の対象となる要素が右側にある場合に現状だとこのように見切れてしまいます。

image

対応としては、要素の位置に応じて自動で Tooltip 自体の位置や矢印の位置が適切になるよう調整するか、ユーザにどの位置に矢印を表示させるか(「左下」や「右下」など)を props で指定させるか、みたいなのが必要そうです!

@AtsushiM AtsushiM changed the title [WIP] feat: add tooltip component feat: add tooltip component Apr 28, 2020
@AtsushiM
Copy link
Member Author

@nabeliwo @Tokky0425
一通り対応しました。ご確認お願いします 🙇

  • 指摘事項
  • iconだとballoonの位置が変わるためicon or それ以外でpositionを変更

Tokky0425
Tokky0425 previously approved these changes Apr 28, 2020
Copy link
Member

@Tokky0425 Tokky0425 left a comment

Choose a reason for hiding this comment

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

確認しました!
Typo だけコメントしてますが、他は LGTM です!😃

src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved
src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@masuP9 masuP9 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/Tooltip/Tooltip.tsx Show resolved Hide resolved
src/components/Tooltip/Tooltip.stories.tsx Show resolved Hide resolved
@AtsushiM
Copy link
Member Author

@Tokky0425 @nabeliwo masuP9さんの指摘内容、対応しましたので改めてreviewお願いします 🙏

Copy link
Member

@Tokky0425 Tokky0425 left a comment

Choose a reason for hiding this comment

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

LGTMです!

@AtsushiM AtsushiM merged commit dc1e2ee into kufu:master May 1, 2020
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.

None yet

4 participants