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

chore: TableコンポーネントのuseReelCells のロジックを整理する #5248

Merged
merged 3 commits into from
Jan 13, 2025
Merged

chore: TableコンポーネントのuseReelCells のロジックを整理する #5248

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc46030
chore: useReelCells内のcurrentRefのチェックを最適化する
AtsushiM Jan 9, 2025
7a47f28
chore: useReelCells内のwindow.ResizeObserverの引数を最適化する
AtsushiM Jan 9, 2025
e11b4ea
chore: useReelCells内のループ処理を最適化する
AtsushiM Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 26 additions & 28 deletions packages/smarthr-ui/src/components/Table/useReelCells.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,42 +7,40 @@ export const useReelCells = () => {
useEffect(() => {
const currentRef = tableWrapperRef.current

if (!currentRef) {
Copy link
Member Author

@AtsushiM AtsushiM Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

currentRefが存在しない限り、後続の処理は不要だったため、ロジックを修正しています
もともとのロジックでは各箇所にcurrentRefの存在チェックがありましたが、分散してしまい見づらかったのでここに集約させます。効率もよいです。

Copy link
Contributor

Choose a reason for hiding this comment

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

早期リターンいいですね 👍

return () => undefined
}

const handleScroll = () => {
if (currentRef) {
const stickyCells = currentRef.querySelectorAll('.fixedElement') || []
const scrollLeft = currentRef.scrollLeft
const maxScrollLeft = currentRef.scrollWidth - currentRef.clientWidth || 0

stickyCells.forEach((cell) => {
const shouldFix = maxScrollLeft > 0 && scrollLeft < maxScrollLeft

if (shouldFix) {
cell.classList.add('fixed')
setShowShadow(scrollLeft > 0)
} else {
cell.classList.remove('fixed')
setShowShadow(maxScrollLeft === 0 && scrollLeft === 0 ? false : true)
}
})
const stickyCells = currentRef.querySelectorAll('.fixedElement')

if (!stickyCells) {
Copy link
Member Author

Choose a reason for hiding this comment

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

stickyCellsが存在しない場合、後続のロジックは不要だったのでifを追加しています

return
}

const scrollLeft = currentRef.scrollLeft
const maxScrollLeft = currentRef.scrollWidth - currentRef.clientWidth || 0
const shouldFix = maxScrollLeft > 0 && scrollLeft < maxScrollLeft
const settableShowShadow = shouldFix
? scrollLeft > 0
: maxScrollLeft !== 0 || scrollLeft !== 0
Comment on lines +23 to +26
Copy link
Member Author

Choose a reason for hiding this comment

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

forEachの中で毎回計算する必要がなかったため、forの外で計算を済ませておきます


stickyCells.forEach((cell) => {
cell.classList.toggle('fixed', shouldFix)
Copy link
Member Author

Choose a reason for hiding this comment

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

classListに対するadd, removeはそれぞれtoggleの第2引数にtrue, falseを渡すことで表現できます。

setShowShadow(settableShowShadow)
})
}
handleScroll()

currentRef?.addEventListener('scroll', handleScroll)
handleScroll()
currentRef.addEventListener('scroll', handleScroll)

const observer = new window.ResizeObserver(() => {
handleScroll()
})
const observer = new window.ResizeObserver(handleScroll)

if (currentRef) {
observer.observe(currentRef)
}
observer.observe(currentRef)

return () => {
if (currentRef) {
currentRef.removeEventListener('scroll', handleScroll)
observer.unobserve(currentRef)
}
currentRef.removeEventListener('scroll', handleScroll)
observer.unobserve(currentRef)
}
}, [tableWrapperRef, setShowShadow])

Expand Down
Loading