Skip to content

fix(backtop & menu): lint, code simplification, deprecated pageYOffset removed #2633

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

Merged
merged 8 commits into from
Oct 11, 2024
Merged

fix(backtop & menu): lint, code simplification, deprecated pageYOffset removed #2633

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6ddcc8b
fix: backtop & menu lint, code simplification, deprecated pageYOffset…
xiaoyatong Oct 10, 2024
9744ce0
fix: add cancelanimation
xiaoyatong Oct 10, 2024
98fa2c7
fix: format
xiaoyatong Oct 10, 2024
e995cb6
fix: fixed by codecr
xiaoyatong Oct 10, 2024
11b9288
fix: test hit
xiaoyatong Oct 10, 2024
b497d1d
fix: test codecr
xiaoyatong Oct 10, 2024
bb1d254
test: fix
xiaoyatong Oct 10, 2024
693adb6
fix: lint of menuitem
xiaoyatong Oct 10, 2024
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
21 changes: 2 additions & 19 deletions src/packages/backtop/backtop.taro.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,14 @@ export const BackTop: FunctionComponent<
...defaultProps,
...props,
}

const classPrefix = 'nut-backtop'

const [backTop, SetBackTop] = useState(false)

const cls = classNames(classPrefix, { show: backTop }, className)
// 监听用户滑动页面事件
usePageScroll((res) => {
const { scrollTop } = res
scrollTop >= threshold ? SetBackTop(true) : SetBackTop(false)
})

// 返回顶部点击事件
const goTop = (e: MouseEvent<HTMLDivElement>) => {
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

性能优化建议

建议使用 useCallback 钩子来优化滚动处理函数，特别是当 threshold 可能经常变化时。这可以防止不必要的重新渲染。例如：

const handleScroll = useCallback((res: { scrollTop: number }) => {
  const { scrollTop } = res
  setBackTop(scrollTop >= threshold)
}, [threshold])

usePageScroll(handleScroll)

这样可以确保只有当 threshold 发生变化时，才会创建新的滚动处理函数。

onClick && onClick(e)
Expand All @@ -47,7 +44,6 @@ export const BackTop: FunctionComponent<
duration: duration > 0 ? duration : 0,
})
}

const styles =
Object.keys(style || {}).length !== 0
? {
Expand All @@ -59,21 +55,8 @@ export const BackTop: FunctionComponent<
bottom: '20px',
zIndex,
}

return (
<div
className={classNames(
classPrefix,
{
show: backTop,
},
className
)}
style={styles}
onClick={(e) => {
goTop(e)
}}
>
<div className={cls} style={styles} onClick={(e) => goTop(e)}>
{children || <Top size={19} className="nut-backtop-main" />}
</div>
)
Expand Down
91 changes: 36 additions & 55 deletions src/packages/backtop/backtop.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
import React, { FunctionComponent, useEffect, useState, useRef } from 'react'
import React, {
FunctionComponent,
useEffect,
useState,
useRef,
useCallback,
} from 'react'
import type { MouseEvent } from 'react'
import { Top } from '@nutui/icons-react'
import classNames from 'classnames'
import { BasicComponent, ComponentDefaults } from '@/utils/typings'
import requestAniFrame from '@/utils/raf'
import requestAniFrame, { cancelRaf } from '@/utils/raf'
import { useRtl } from '@/packages/configprovider'

declare const window: any

export interface BackTopProps extends BasicComponent {
target: string
threshold: number
Expand Down Expand Up @@ -46,33 +50,37 @@
const [backTop, SetBackTop] = useState(false)
const [scrollTop, SetScrollTop] = useState(0)
let startTime = 0
const scrollEl: any = useRef<any>(null)
useEffect(() => {
init()
return () => removeEventListener()
}, [])
const scrollEl = useRef<any>(null)
const cls = classNames(classPrefix, { show: backTop }, className)

const scrollListener = useCallback(() => {
let top = null
if (scrollEl.current instanceof Window) {
top = scrollEl.current.scrollY
} else {
top = scrollEl.current?.scrollTop
}
SetScrollTop(top)
SetBackTop(top >= threshold)

Check warning on line 64 in src/packages/backtop/backtop.tsx

View check run for this annotation

Codecov / codecov/patch

src/packages/backtop/backtop.tsx#L57-L64 

Added lines #L57 - L64 were not covered by tests
}, [threshold])

const init = () => {
const init = useCallback(() => {
if (target && document.getElementById(target)) {
scrollEl.current = document.getElementById(target) as HTMLElement | Window
scrollEl.current = document.getElementById(target)

Check warning on line 69 in src/packages/backtop/backtop.tsx

View check run for this annotation

Codecov / codecov/patch

src/packages/backtop/backtop.tsx#L69 

Added line #L69 was not covered by tests
} else {
scrollEl.current = window
}
addEventListener()
initCancelAniFrame()
}
const scrollListener = () => {
let top: any = null
if (scrollEl.current instanceof Window) {
top = scrollEl.current.pageYOffset
SetScrollTop(top)
} else {
top = scrollEl.current.scrollTop
SetScrollTop(top)
scrollEl.current?.addEventListener('scroll', scrollListener, false)
scrollEl.current?.addEventListener('resize', scrollListener, false)
}, [target, scrollListener])

useEffect(() => {
init()
return () => {
scrollEl.current?.removeEventListener('scroll', scrollListener, false)
scrollEl.current?.removeEventListener('resize', scrollListener, false)
}
const showBtn = top >= threshold
SetBackTop(showBtn)
}
}, [init, scrollListener])

const scroll = (y = 0) => {
if (scrollEl.current instanceof Window) {
Expand All @@ -90,29 +98,14 @@
scroll(y)
cid = requestAniFrame(fn)
if (t === duration || y === 0) {
window.cancelAnimationFrame(cid)
cancelRaf(cid)

Check warning on line 101 in src/packages/backtop/backtop.tsx

View check run for this annotation

Codecov / codecov/patch

src/packages/backtop/backtop.tsx#L101 

Added line #L101 was not covered by tests
}
})
}

const initCancelAniFrame = () => {
window.cancelAnimationFrame = window.webkitCancelAnimationFrame
}

function addEventListener() {
scrollEl.current?.addEventListener('scroll', scrollListener, false)
scrollEl.current?.addEventListener('resize', scrollListener, false)
}

function removeEventListener() {
scrollEl.current?.removeEventListener('scroll', scrollListener, false)
scrollEl.current?.removeEventListener('resize', scrollListener, false)
}

const goTop = (e: MouseEvent<HTMLDivElement>) => {
onClick && onClick(e)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议使用可选链调用onClick函数。

可以将onClick && onClick(e)替换为onClick?.(e)，使代码更简洁。

应用以下修改：

-    onClick && onClick(e)
+    onClick?.(e)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
onClick && onClick(e)
onClick?.(e)
🧰 Tools
🪛 Biome

[error] 107-107: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

const otime = +new Date()
startTime = otime
startTime = +new Date()

Check warning on line 108 in src/packages/backtop/backtop.tsx

View check run for this annotation

Codecov / codecov/patch

src/packages/backtop/backtop.tsx#L108 

Added line #L108 was not covered by tests
duration > 0 ? scrollAnimation() : scroll()
}

Expand All @@ -129,19 +122,7 @@
}

return (
<div
className={classNames(
classPrefix,
{
show: backTop,
},
className
)}
style={styles}
onClick={(e) => {
goTop(e)
}}
>
<div className={cls} style={styles} onClick={(e) => goTop(e)}>
{children || <Top width={19} height={19} className="nut-backtop-main" />}
</div>
)
Expand Down
34 changes: 17 additions & 17 deletions src/packages/menu/menu.taro.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import React, { FunctionComponent, useEffect, useRef, useState } from 'react'
import React, {
FunctionComponent,
useCallback,
useEffect,
useRef,
useState,
} from 'react'
import classNames from 'classnames'
import { ArrowDown, ArrowUp } from '@nutui/icons-react-taro'
import { OptionItem, MenuItem } from '@/packages/menuitem/menuitem.taro'
Expand Down Expand Up @@ -53,29 +59,30 @@ export const Menu: FunctionComponent<Partial<MenuProps>> & {
...props,
}
const menuRef = useRef(null)
const [showMenuItem, setShowMenuItem] = useState<boolean[]>([])
const [menuItemTitle, setMenuItemTitle] = useState<string[]>([])
const [isScrollFixed, setIsScrollFixed] = useState(false)
const cls = classNames(`nut-menu`, className, {
'scroll-fixed': isScrollFixed,
})

const getScrollTop = (el: Element | Window) => {
return Math.max(0, 'scrollTop' in el ? el.scrollTop : el.pageYOffset)
return Math.max(0, 'scrollTop' in el ? el.scrollTop : el.scrollY)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

修复 getScrollTop 函数的兼容性问题

在 getScrollTop 函数中：

return Math.max(0, 'scrollTop' in el ? el.scrollTop : el.scrollY)

当 el 为元素时，el.scrollTop 存在，但 el.scrollY 不存在；当 el 为 window 时，el.scrollY 存在，但 'scrollTop' in el 判断会返回 false，导致获取 el.scrollY，这在某些浏览器会有兼容性问题。

建议修改为：

- return Math.max(0, 'scrollTop' in el ? el.scrollTop : el.scrollY)
+ return Math.max(0, el === window ? window.scrollY : (el as Element).scrollTop)

这样可以准确区分 el 是否为 window，并获取正确的滚动位置，增强了函数的兼容性。

}
const onScroll = () => {
const { scrollFixed } = props

const onScroll = useCallback(() => {
const scrollTop = getScrollTop(window)
const isFixed =
scrollTop > (typeof scrollFixed === 'boolean' ? 30 : Number(scrollFixed))
setIsScrollFixed(isFixed)
}
}, [scrollFixed])

useEffect(() => {
if (scrollFixed) {
window.addEventListener('scroll', onScroll)
}
return () => window.removeEventListener('scroll', onScroll)
}, [])
}, [scrollFixed, onScroll])

const [showMenuItem, setShowMenuItem] = useState<boolean[]>([])
const [menuItemTitle, setMenuItemTitle] = useState<string[]>([])
const toggleMenuItem: MenuCallBackFunction = (index, from = 'NORMAL') => {
showMenuItem[index] = !showMenuItem[index]
if (showMenuItem[index]) {
Expand All @@ -97,7 +104,6 @@ export const Menu: FunctionComponent<Partial<MenuProps>> & {
menuItemTitle[index] = text
setMenuItemTitle([...menuItemTitle])
}

const cloneChildren = () => {
return React.Children.map(children, (child, index) => {
return React.cloneElement(child as any, {
Expand Down Expand Up @@ -179,13 +185,7 @@ export const Menu: FunctionComponent<Partial<MenuProps>> & {
})
}
return (
<div
{...rest}
className={classNames(`nut-menu`, className, {
'scroll-fixed': isScrollFixed,
})}
ref={menuRef}
>
<div {...rest} className={cls} ref={menuRef}>
<div
className={classNames('nut-menu-bar', {
opened: showMenuItem.includes(true),
Expand Down
33 changes: 17 additions & 16 deletions src/packages/menu/menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import React, { FunctionComponent, useEffect, useRef, useState } from 'react'
import React, {
FunctionComponent,
useCallback,
useEffect,
useRef,
useState,
} from 'react'
import classNames from 'classnames'
import { ArrowDown, ArrowUp } from '@nutui/icons-react'
import { OptionItem, MenuItem } from '@/packages/menuitem/menuitem'
Expand Down Expand Up @@ -53,29 +59,30 @@
...props,
}
const menuRef = useRef(null)
const [showMenuItem, setShowMenuItem] = useState<boolean[]>([])
const [menuItemTitle, setMenuItemTitle] = useState<string[]>([])
const [isScrollFixed, setIsScrollFixed] = useState(false)
const cls = classNames(`nut-menu`, className, {
'scroll-fixed': isScrollFixed,
})

const getScrollTop = (el: Element | Window) => {
return Math.max(0, 'scrollTop' in el ? el.scrollTop : el.pageYOffset)
return Math.max(0, 'scrollTop' in el ? el.scrollTop : el.scrollY)

Check warning on line 70 in src/packages/menu/menu.tsx

View check run for this annotation

Codecov / codecov/patch

src/packages/menu/menu.tsx#L70 

Added line #L70 was not covered by tests
}
const onScroll = () => {
const { scrollFixed } = props

const onScroll = useCallback(() => {
const scrollTop = getScrollTop(window)
const isFixed =
scrollTop > (typeof scrollFixed === 'boolean' ? 30 : Number(scrollFixed))
setIsScrollFixed(isFixed)
}
}, [scrollFixed])
Comment on lines +75 to +80
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

确保scrollFixed属性的类型安全性。

在onScroll函数中，Number(scrollFixed)可能会返回NaN，如果scrollFixed不是一个可转换为数字的字符串。建议在使用Number(scrollFixed)之前，验证scrollFixed是否为有效数字，以避免潜在的问题。

建议在使用前添加类型检查和默认值，例如：

const isFixed =
-  scrollTop > (typeof scrollFixed === 'boolean' ? 30 : Number(scrollFixed))
+  scrollTop > (typeof scrollFixed === 'boolean'
+    ? 30
+    : !isNaN(Number(scrollFixed))
+    ? Number(scrollFixed)
+    : 30)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const onScroll = useCallback(() => {
const scrollTop = getScrollTop(window)
const isFixed =
scrollTop > (typeof scrollFixed === 'boolean' ? 30 : Number(scrollFixed))
setIsScrollFixed(isFixed)
}
}, [scrollFixed])
const onScroll = useCallback(() => {
const scrollTop = getScrollTop(window)
const isFixed =
scrollTop > (typeof scrollFixed === 'boolean'
? 30
: !isNaN(Number(scrollFixed))
? Number(scrollFixed)
: 30)
setIsScrollFixed(isFixed)
}, [scrollFixed])


useEffect(() => {
if (scrollFixed) {
window.addEventListener('scroll', onScroll)
}
return () => window.removeEventListener('scroll', onScroll)
}, [])
}, [scrollFixed, onScroll])

const [showMenuItem, setShowMenuItem] = useState<boolean[]>([])
const [menuItemTitle, setMenuItemTitle] = useState<string[]>([])
const toggleMenuItem: MenuCallBackFunction = (index, from = 'NORMAL') => {
showMenuItem[index] = !showMenuItem[index]
if (showMenuItem[index]) {
Expand Down Expand Up @@ -180,13 +187,7 @@
})
}
return (
<div
{...rest}
className={classNames(`nut-menu`, className, {
'scroll-fixed': isScrollFixed,
})}
ref={menuRef}
>
<div {...rest} className={cls} ref={menuRef}>
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

合并rest中的className以避免覆盖

在第193行，<div>元素使用了{...rest}扩展属性并指定了className={cls}。如果rest中包含className属性，它将被cls覆盖。建议合并rest.className和cls，以确保所有传入的类名都被正确应用。

您可以按以下方式修改代码：

-        <div {...rest} className={cls} ref={menuRef}>
+        <div {...rest} className={classNames(cls, rest.className)} ref={menuRef}>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<div {...rest} className={cls} ref={menuRef}>
<div {...rest} className={classNames(cls, rest.className)} ref={menuRef}>

<div
className={classNames('nut-menu-bar', {
opened: showMenuItem.includes(true),
Expand Down
5 changes: 3 additions & 2 deletions src/utils/raf.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

// 防频
function requestAniFrame() {
if (typeof window !== 'undefined') {
if (inBrowser) {
const _window = window as any
return (
_window.requestAnimationFrame ||
Expand All @@ -19,7 +19,8 @@

export function cancelRaf(id: number) {
if (inBrowser) {
cancelAnimationFrame(id)
const _window = window as any
;(_window.cancelAnimationFrame || _window.webkitCancelAnimationFrame)(id)

Check warning on line 23 in src/utils/raf.ts

View check run for this annotation

Codecov / codecov/patch

src/utils/raf.ts#L22-L23 

Added lines #L22 - L23 were not covered by tests
} else {
clearTimeout(id)
}
Expand Down
Loading