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 follow button on browse testimony page #1632

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LauraUmana
Copy link
Collaborator

Issue: #1621

The changes I made were:

  1. Added an event prevent default so it would no longer navigate to the next page
  2. I had to add a context to the follow button so I can bring state up.
  3. I also changed the queryResult of the useEffect in FollowButton.tsx to an object instead of a string. This way it can house all of the orgs I'm following and not following.

Checklist

  • [ x] On the frontend, I've made my strings translate-able.

Screenshots

Screenshot 2024-10-08 at 2 48 05 AM Screenshot 2024-10-08 at 2 49 43 AM

#Known Issues
N/A

#Steps to test/reproduce

  1. Go to the home page
  2. Click on Browse Testimony
  3. Click on a follow button for an organization like Pythagorean Pizza society

Copy link

vercel bot commented Oct 8, 2024

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

Name Status Preview Comments Updated (UTC)
maple-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 17, 2024 4:42am

Copy link
Collaborator

@kiminkim724 kiminkim724 left a comment

Choose a reason for hiding this comment

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

This is a great start! I just had a few comments:

  • One thing to note is that the FollowButton component is also being used to follow Bills on each Bill Detail Page with the FollowBillButton component in the same file.
    • As a result, the Follow button doesn't work on the Bill Detail page, which I assume is because the Context Provider is being applied only for TestimonySearch, which does not cover the Bill Detail page.
    • A solution could be that we could move the Provider higher up the component tree so that both the Bill Detail Page and the Testimony Search are included in the provider.
  • There are also a few formatting errors (i.e. spacing)

@LauraUmana
Copy link
Collaborator Author

@kiminkim724 Hi Kimin! Thanks so much for the feedback. I made the changes to the PR.

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.

2 participants