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

freeze admin bounty overview section #1243

Merged
merged 7 commits into from
Jan 8, 2024
Merged

freeze admin bounty overview section #1243

merged 7 commits into from
Jan 8, 2024

Conversation

Ekep-Obasi
Copy link
Contributor

Describe your changes

Freeze bounty overview on scroll using IntersectionObserver

  • Intersection Observer: Used to trigger callback when scrolling past reference component.
  • Custom Hook: useInViewPort encapsulates Intersection Observer logic for reusability.

Reason for Intersection Observer:

  • CSS display: sticky; not applicable due to parent component's overflow property.

Issue ticket number and link #1014

Preview

https://www.loom.com/share/bf450b3c9b6d4a778a1f2c32184b57bd?sid=7662d832-11e2-40a4-b5dc-15bf069aceec

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested on Chrome and Firefox
  • I have provided a screenshot or recording of changes in my PR if there were updates to the frontend

- wrapped intersection observer logic in a custom hook
- hook return a boolean that tells whether or not an node is in view
- hook also return a ref for the node we are observing
@ecurrencyhodler
Copy link
Contributor

Hey @Ekep-Obasi it looks like two of your tests are failing. Can you take another look? Let us know if you need some help.

@Ekep-Obasi
Copy link
Contributor Author

Yes @ecurrencyhodler
I need some help with this

@ecurrencyhodler
Copy link
Contributor

@kevkevinpal @elraphty can you give some insight here?

@elraphty
Copy link
Contributor

elraphty commented Jan 5, 2024

@kevkevinpal @elraphty can you give some insight here?

@Ekep-Obasi your prettier is also failing to run yarn run prettier to fix, the issue is you added some code that increased the lines of code we have, but the tests were not increased and there was a threshold set. The error Jest: "./src/hooks/" coverage threshold for lines (37.03%) not met: 35.1%, add some test for .src/hooks to meet the coverage threshold.

@Ekep-Obasi
Copy link
Contributor Author

@elraphty, Do you mean that I should write tests for the component I just created?

And for the prettier error in ./src/hooks/useInViewport.ts I've ran yarn run prettier and even ran npx prettier --config .prettierrc.json --check ./src/hooks/useInViewport.ts to check. But the prettier workflow still seems to be failing

Capture

@kevkevinpal
Copy link
Contributor

@elraphty, Do you mean that I should write tests for the component I just created?

And for the prettier error in ./src/hooks/useInViewport.ts I've ran yarn run prettier and even ran npx prettier --config .prettierrc.json --check ./src/hooks/useInViewport.ts to check. But the prettier workflow still seems to be failing

Capture

I would create a test that covers useInViewPort I wouldn't worry about the prettier check as much as the jest test check

@Ekep-Obasi
Copy link
Contributor Author

@elraphty, Do you mean that I should write tests for the component I just created?
And for the prettier error in ./src/hooks/useInViewport.ts I've ran yarn run prettier and even ran npx prettier --config .prettierrc.json --check ./src/hooks/useInViewport.ts to check. But the prettier workflow still seems to be failing
Capture

I would create a test that covers useInViewPort I wouldn't worry about the prettier check as much as the jest test check

@kevkevinpal Thanks for the help!

@Ekep-Obasi
Copy link
Contributor Author

@kevkevinpal @elraphty
After adding tests for the hook and running yarn run test locally all tests pass

Capture

But the jest workflow is failing with this error

Jest worker encountered 4 child process exceptions, exceeding retry limit

top: 62px;
left: 0;
width: 100%;
z-index: 9999999;
Copy link
Contributor

Choose a reason for hiding this comment

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

this z-index doesnt feel like best practice is there a style to keep it always ontop? if you can look into alternatives to doing this that would be good

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just going to make this another issue so we can merge this PR for now

@ecurrencyhodler
Copy link
Contributor

Tested on staging and it looks good! Paid.

@ecurrencyhodler
Copy link
Contributor

Increased it to 300k for you.

@Ekep-Obasi
Copy link
Contributor Author

@ecurrencyhodler thank you 🙏

elraphty pushed a commit that referenced this pull request Jan 26, 2024
* feat: created useInViewPort custom hook

- wrapped intersection observer logic in a custom hook
- hook return a boolean that tells whether or not an node is in view
- hook also return a ref for the node we are observing

* feat: freeze bounties overview section

* refactor: refactored proptypes

* test: wrote tests for useInViewport hook

* feat: freeze bounties overview section
elraphty pushed a commit that referenced this pull request Jan 26, 2024
* feat: created useInViewPort custom hook

- wrapped intersection observer logic in a custom hook
- hook return a boolean that tells whether or not an node is in view
- hook also return a ref for the node we are observing

* feat: freeze bounties overview section

* refactor: refactored proptypes

* test: wrote tests for useInViewport hook

* feat: freeze bounties overview section
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.

4 participants