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

Fix/comment expand error #4502

Merged
merged 15 commits into from
May 31, 2024
Merged

Fix/comment expand error #4502

merged 15 commits into from
May 31, 2024

Conversation

wlliaml
Copy link
Contributor

@wlliaml wlliaml commented May 30, 2024

No description provided.

@wlliaml wlliaml requested a review from a team as a code owner May 30, 2024 09:36
Copy link

vercel bot commented May 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
matters-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2024 10:47am


export const checkIsSafariVersionLessThan17 = () => {
const userAgent = navigator.userAgent
const safariMatch = userAgent.match(/Version\/(\d+\.\d+)/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a better RegExp to match Safari.

Here is one from GPT:

/^Mozilla\/5\.0 \(Macintosh; Intel Mac OS X \d+_\d+(?:_\d+)?\) AppleWebKit\/\d+(?:\.\d+)* \(KHTML, like Gecko\) Version\/\d+\.\d+(?:\.\d+)* Safari\/\d+(?:\.\d+)*$/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not applicable to Apple M series chips?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is mine: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.5 Safari/605.1.15

collapseable = true,
bgColor = 'white',
}) => {
const [expandable, setExpandable] = useState(false)
const [firstRender, setFirstRender] = useState(true)
const [isOverFlowing, setIsOverFlowing] = useState(false)
const [expand, setExpand] = useState(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to expanded / setExpanded or isExpanded / setIsExpanded would be clearer

if (!node?.current) {
return
}
if (checkOverflow(node.current, limit, buffer, isRichShow, isComment)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is only one place to use checkOverflow, could we move it back to handleOverflowCheck to avoid unnecessary props passing?

if (isSafariVersionLessThan17 && isRichShow) {
reset()
setIsRichShow(false)
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should avoid using setTimeout, can add another useEffect hook or merge it into L148 instead:

useEffect(() => {
    handleOverflowCheck()
}, [...])

or

useIsomorphicLayoutEffect(() => {
  reset()
  handleOverflowCheck()
}, [content, isRichShow])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use async function instead of setTimeout

@wlliaml wlliaml requested a review from robertu7 May 31, 2024 08:11
const [firstRender, setFirstRender] = useState(true)
const [expand, setExpand] = useState(true)
const [isOverFlowing, setIsOverFlowing] = useState(false)
const [isExpand, setIsExpand] = useState(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Expand => Expanded

@wlliaml wlliaml merged commit d51bc39 into develop May 31, 2024
5 of 6 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fix/comment-expand-error branch May 31, 2024 11:50
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.

Issue related to comment expandable button
2 participants