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

chore: DatePickerの内部処理を整理する #5329

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

AtsushiM
Copy link
Member

関連URL

概要

変更内容

確認方法

@yagimushi yagimushi force-pushed the chore-refactoring-DatePicker branch 3 times, most recently from c38d089 to 35a48fe Compare January 24, 2025 04:03
@yagimushi yagimushi force-pushed the chore-refactoring-DatePicker branch from 35a48fe to 8d05c8b Compare January 24, 2025 04:04
Copy link

pkg-pr-new bot commented Jan 24, 2025

Open in Stackblitz

npm i https://pkg.pr.new/kufu/smarthr-ui@5329

commit: bd555c4

Comment on lines +17 to +20
// has no space on bottom side
inputRect.bottom + contentHeihgt > innerHeight &&
// top side space bigger than bottom side
inputRect.top > innerHeight - inputRect.bottom
Copy link
Member Author

Choose a reason for hiding this comment

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

元々は変数化されていたのでifの実行前に余計な計算がされる可能性がありました。
if条件内に移動することで必要最小限の実行になるように修正しています

Comment on lines +41 to +49
const style = useMemo(() => portal(), [])

const styleAttr = useMemo(
() => ({
top: `${position.top}px`,
left: `${position.left}px`,
}),
[position.left, position.top],
)
Copy link
Member Author

Choose a reason for hiding this comment

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

不必要にtailwindの実行がされる可能性があったため、memoを分割しています

if (!containerRef.current) {
return
if (containerRef.current) {
setPosition(getPortalPosition(inputRect, containerRef.current.offsetHeight))
Copy link
Member Author

Choose a reason for hiding this comment

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

直後の処理の条件の逆の早期returnになっていたため、整理しています

const firstCalendarButton = calendarButtons[0]
const lastCalendarButton = calendarButtons[calendarButtons.length - 1]
Copy link
Member Author

Choose a reason for hiding this comment

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

直後のifで早期returnされる場合、利用されない変数だったため、位置を移動しています


const switchCalendarVisibility = useCallback((isVisible: boolean) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

明確にtrue, falseを渡す箇所が多く、依存関係の処理などがわかりにくくなっていたため、open, closeを明確に行う別関数を用意しました

@AtsushiM AtsushiM marked this pull request as ready for review January 24, 2025 04:22
@AtsushiM AtsushiM requested a review from a team as a code owner January 24, 2025 04:22
@AtsushiM AtsushiM requested review from oti and yuzuru-akiyama and removed request for a team January 24, 2025 04:22
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.

1 participant