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

Add browse species button unconditionally #1345

Merged

Conversation

jasonwang7517
Copy link
Contributor

🔗 Issue

Original issue

✍️ Description

  • Changes logic for creation of Browse buttons on the root page to be programmatic based on unique species

📷 Screenshots/Demos

@jasonwang7517 jasonwang7517 marked this pull request as ready for review January 25, 2025 02:29
@jasonwang7517
Copy link
Contributor Author

@kasugaijin I know there was a conversation on the original PR about whether we should only display the button if there are adoptable pets of that given species but I know we also added an empty state.

Thus, I kept the scope as is to just pull the unique species of all pets regardless of placement_type value but if you would like me to specify the scope to factor that in as well such that, for example, Browse Dogs only appears if there is at least one record with species: "Dog" and placement_type: "Adoptable" just let me know.

@kasugaijin
Copy link
Collaborator

@kasugaijin I know there was a conversation on the original PR about whether we should only display the button if there are adoptable pets of that given species but I know we also added an empty state.

Thus, I kept the scope as is to just pull the unique species of all pets regardless of placement_type value but if you would like me to specify the scope to factor that in as well such that, for example, Browse Dogs only appears if there is at least one record with species: "Dog" and placement_type: "Adoptable" just let me know.

The simplest approach is to keep as you suggest. I just wonder though, why take people to an empty page?

@jasonwang7517
Copy link
Contributor Author

@kasugaijin I know there was a conversation on the original PR about whether we should only display the button if there are adoptable pets of that given species but I know we also added an empty state.
Thus, I kept the scope as is to just pull the unique species of all pets regardless of placement_type value but if you would like me to specify the scope to factor that in as well such that, for example, Browse Dogs only appears if there is at least one record with species: "Dog" and placement_type: "Adoptable" just let me know.

The simplest approach is to keep as you suggest. I just wonder though, why take people to an empty page?

To get their hopes of adopting their dream pet as high as we can before we shatter them mercilessly.

Alternatively, we can add some more logic in the partial to disable the button if there aren't any adoptable pets of that species. Just an initial suggestion. Or we can keep it as is, totally up to you boss.

@kasugaijin
Copy link
Collaborator

kasugaijin commented Jan 25, 2025

@kasugaijin I know there was a conversation on the original PR about whether we should only display the button if there are adoptable pets of that given species but I know we also added an empty state.
Thus, I kept the scope as is to just pull the unique species of all pets regardless of placement_type value but if you would like me to specify the scope to factor that in as well such that, for example, Browse Dogs only appears if there is at least one record with species: "Dog" and placement_type: "Adoptable" just let me know.

The simplest approach is to keep as you suggest. I just wonder though, why take people to an empty page?

To get their hopes of adopting their dream pet as high as we can before we shatter them mercilessly.

Alternatively, we can add some more logic in the partial to disable the button if there aren't any adoptable pets of that species. Just an initial suggestion. Or we can keep it as is, totally up to you boss.

Can’t we just chain two scopes together? I think we have one to get the adoptable pets, then one to get the species? Then the whole thing is controlled by one query without need for additional view logic. Does that make sense?

@jasonwang7517
Copy link
Contributor Author

Yeah we totally could, wasn't sure if you wanted to separate the logic but I'll chain 'em together. Say no more.

app/models/pet.rb Outdated Show resolved Hide resolved
@kasugaijin
Copy link
Collaborator

kasugaijin commented Jan 29, 2025

Looking good. There’s one more place we use these buttons - log in as an adopter or fosterer and in your dashboard index you will see them. Can you use this feature there as well?

also if you want to add in this PR we also need to add the logic to the top navbar dropdown that lets your browse species. In this case I think it’s just normal link to without button styling.

@jasonwang7517
Copy link
Contributor Author

Gotchu

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

It's looking good! Sorry for my back and forth on it. These last two small changes should do it for approval.

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

Nice!

@kasugaijin kasugaijin merged commit 1376aef into rubyforgood:main Jan 30, 2025
5 checks passed
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