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

<Fade />作ってみた #179

Merged
merged 14 commits into from
Oct 1, 2020
Merged

<Fade />作ってみた #179

merged 14 commits into from
Oct 1, 2020

Conversation

youchann
Copy link
Contributor

@youchann youchann commented Sep 17, 2020

イメージはこれ
https://material-ui.com/components/transitions/#fade

やったこと

  • <Fade />, <Backdrop />の追加
  • <Modal />の改造(onClose()propsを入れられるように)
  • ↑を起点にいろんなコンポーネントを芋づる式で修正

@youchann youchann added the enhancement New feature or request label Sep 17, 2020
@youchann youchann self-assigned this Sep 17, 2020
@youchann youchann changed the title <BackDrop />作ってみた <LoadingOverlay />作ってみた Sep 17, 2020
@youchann
Copy link
Contributor Author

アンマウント時にもアニメーションがつけられないか考える

@youchann
Copy link
Contributor Author

今の実装だと単一のコンポーネントだけアニメーションを変えたい。みたいな要望に対応できないので、責務を分散させる。

@youchann youchann changed the title <LoadingOverlay />作ってみた <Fade />作ってみた Sep 28, 2020
@youchann
Copy link
Contributor Author

多分これでいい感じなはず

fadeProps?: FadeProps;
};

const Backdrop = React.forwardRef<HTMLDivElement, BackdropProps>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<Backdrop />はただのposition: fixed;な背景。
フェードイン・アウトもできる。

Comment on lines +71 to +72
<Modal isOpen={isOpen} enableTransition={true}>
<Fade in={isOpen}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

変更量多く見えるけど、上記2つのコンポーネントが変わっただけ。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

変更後は利用側でネストが浅くなる

{isOpen & (
  <ConfirmModal />
)}

// ↓こう書けば良くなる
<ConfirmModal isOpen={isOpen} />

Comment on lines -39 to -40
onClick={onHandleToggleContent(false)}
onClickAway={onHandleToggleContent(false)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

破壊的変更

onClickonClickAwayでハンドリングするのではなくonCloseだけでハンドリングできるようになった。


return (
<Portal>
<Styled.Container isHidden={!isOpen && exited}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isOpenがralseになる→<Backdrop />のアニメーションが終わる→exitedがtrueになる→<Modal />visibility: hidden;になる。

isOpen={isOpen}
backdropProps={{ invisible: true }}
onClose={onClose}
>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここの1行下に<Fade />とかを入れられるようにして、Popoverにもtransitionを入れられるようにする予定

children: React.ReactNode;
onClose?: (
event: React.MouseEvent<HTMLDivElement, MouseEvent>,
reason: ModalCloseReason | FloatingTipCloseReason,
Copy link
Contributor

Choose a reason for hiding this comment

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

このreasonってどんな理由で設定されているものなんですか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

例えば<FloatingTip />でいうと、「ポップアップで出てくる要素外のクリックでは閉じないけど、✖️アイコンのクリックでは閉じたい」みたいなユースケースに対応するためですね!

Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど、onClose に渡すメソッドが reason の内容によって振る舞いを変えることができるようになるわけですね

@@ -83,8 +83,7 @@ export const Overview: React.FunctionComponent = () => {
baseElement={buttonElement}
contents={contents}
positionPriority={[position]}
onClick={onHandleToggleMenu(false)}
onClickAway={onHandleToggleMenu(false)}
onClose={onHandleToggleMenu(false)}
Copy link
Contributor

@maktak1995 maktak1995 Oct 1, 2020

Choose a reason for hiding this comment

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

そうか、reason があるから onClickonClickAwayonClose にまとめられるのですね

Copy link
Contributor

@maktak1995 maktak1995 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 fe759b9 into master Oct 1, 2020
@youchann youchann deleted the add-backdrop branch October 1, 2020 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants