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.

Sign up for GitHub

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

Jump to bottom

<FloatingTip />の実装 #67

Merged
merged 16 commits into from
Jun 16, 2020
Merged

<FloatingTip />の実装 #67

merged 16 commits into from
Jun 16, 2020
+451 −70

Conversation

youchann
Copy link
Contributor

@youchann youchann commented Jun 9, 2020

close: #61

@youchann youchann added the enhancement New feature or request label Jun 9, 2020
@youchann youchann self-assigned this Jun 9, 2020
@youchann
Copy link
Contributor Author

youchann commented Jun 9, 2020

TODO

  • 未マージとなっている機能を使っているので、それらがマージされるまで待つ

    • <ClickAwayListener />
    • <Modal />

  • Issueに記載した設計で問題ないか確認&�<Menu />に適用する

@youchann
Copy link
Contributor Author

とりあえず作り切ったので、命名周りを相談する

@youchann
Copy link
Contributor Author

https://cartaholdings.slack.com/archives/CKYAWNNLX/p1591862578167400

<Floater />
<FloatingTip />
<FloatingPanel />

3つの候補から<FloatingTip />に決定した。

@youchann youchann changed the title wip <Popover />の実装 <FloatingTip />の実装 Jun 11, 2020
@kinokoruumu
Copy link
Contributor

TODO

  • 未マージとなっている機能を使っているので、それらがマージされるまで待つ

    • <ClickAwayListener />
    • <Modal />

  • Issueに記載した設計で問題ないか確認&�<Menu />に適用する

こういうのはdescriptionにcheckboxとかで書いてもらえると終わったかどうか分かりやすい！

kinokoruumu
kinokoruumu reviewed Jun 16, 2020
View reviewed changes
src/components/FloatingTip/FloatingTip.tsx Outdated
const FloatingTip: React.FunctionComponent<Props> = ({
baseElement,
positionPriority = ["auto"],
offset = [0, 0],
Copy link
Contributor

Choose a reason for hiding this comment

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

yは10?くらい当てといて良さそう！
positionPriorityも利用頻度が一番高いところ(カンプがあればそれに近いところ)を入れておくと使いやすそう
right_startとかかな？

src/components/FloatingTip/FloatingTip.tsx Outdated
baseElement: HTMLElement | null;
positionPriority?: PopperJS.Placement[];
offset?: [number, number];
open: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
open: boolean;
isOpen: boolean;

命名は他に合わせてもらえると！

@kinokoruumu
Copy link
Contributor

このPR終わったらDstへの導入もしてくれる感じかな？
@maktak1995 にアサインしてくれても大丈夫！
既存コンポーネントをingred-uiに移行したあとは２重管理にならないよう早めにDstから消したい！

@youchann
Copy link
Contributor Author

自分がToastやって、まきたけさんが旧Popoverをやる感じでいきますか!!!

kinokoruumu
kinokoruumu approved these changes Jun 16, 2020
View reviewed changes
Copy link
Contributor

@kinokoruumu kinokoruumu left a comment

Choose a reason for hiding this comment

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

LGTMです！

@youchann youchann merged commit d63a361 into master Jun 16, 2020
@youchann youchann deleted the add-popover branch June 16, 2020 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinokoruumu kinokoruumu kinokoruumu approved these changes

Assignees

@youchann youchann

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Popoverを実装
2 participants
@youchann @kinokoruumu