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

feat: 🎸 Add See More Section #428

Merged
merged 16 commits into from
Aug 20, 2024
Merged

Conversation

Malayvasa
Copy link
Contributor

Related to #384

Comment on lines 32 to 43
useEffect(() => {
if (!containerRef.current) return

const updateWidth = (entries: ResizeObserverEntry[]) => {
setWidth(entries[0].contentRect.width)
}

const resizeObserver = new ResizeObserver(updateWidth)
resizeObserver.observe(containerRef.current)

return () => resizeObserver.disconnect()
}, [])
Copy link
Member

Choose a reason for hiding this comment

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

Nit: let's move this to a custom hook. We could make it reusable and return the width and height.


pathRef.current?.setAttribute('d', `M${path}`)

requestRef.current = requestAnimationFrame(animate)
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to use a ref for this. We can use a local variable.

@Malayvasa Malayvasa requested a review from iamacook August 1, 2024 11:15
@Malayvasa Malayvasa changed the base branch from malay-branch-1 to main August 2, 2024 15:15
src/components/DataRoom/SeeMore/index.tsx Outdated Show resolved Hide resolved
src/components/DataRoom/SeeMore/styles.module.css Outdated Show resolved Hide resolved
src/components/DataRoom/SeeMore/styles.module.css Outdated Show resolved Hide resolved
src/hooks/useContainerSize.ts Outdated Show resolved Hide resolved
Copy link
Member

@DiogoSoaress DiogoSoaress left a comment

Choose a reason for hiding this comment

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

Good job! There is one last comment but it should be good to merge after


const pathElement = containerRef.current?.querySelector('path')
if (pathElement) {
pathElement.setAttribute('d', `M${path}`)
Copy link
Member

Choose a reason for hiding this comment

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

I am having a runtime error coming from this line
Screenshot 2024-08-12 at 12 22 25

@Malayvasa Malayvasa requested a review from DiogoSoaress August 13, 2024 08:51
Copy link
Member

@DiogoSoaress DiogoSoaress left a comment

Choose a reason for hiding this comment

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

I was also observing huge CPU usage in this branch and in the Wave.tsx file the sin computation and rendering is running as fast as possible the whole time…. We should try to only compute once the sin waves values or load them from a JSON file

@Malayvasa Malayvasa requested a review from DiogoSoaress August 20, 2024 07:54
@Malayvasa
Copy link
Contributor Author

I have moved the implementation of the Wave Animation to PR #441 as it is not yet performant.

Copy link
Member

@DiogoSoaress DiogoSoaress left a comment

Choose a reason for hiding this comment

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

Some leftovers

Comment on lines 30 to 32
{
//Moved the wave animation to PR #441
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
//Moved the wave animation to PR #441
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was previously addressed in 44b8b93

src/components/DataRoom/SeeMore/SlidingWave.tsx Outdated Show resolved Hide resolved
src/components/DataRoom/SeeMore/SlidingWave.tsx Outdated Show resolved Hide resolved
src/components/DataRoom/SeeMore/styles.module.css Outdated Show resolved Hide resolved
@Malayvasa Malayvasa requested a review from DiogoSoaress August 20, 2024 08:47
@DiogoSoaress DiogoSoaress merged commit 0d455d3 into safe-global:main Aug 20, 2024
3 of 4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants