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: #1130, use gradesObj in TickButton instead of deprecated yds property #1193

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

julianlam
Copy link
Contributor

@julianlam julianlam commented Oct 31, 2024

Observed in Production

  • Certain climbs are un-tickable. Attempting to tick them causes the TickForm to display the following error (image courtesy of @emilyg1709):
  • It was also observed that some climbs are tickable, namely older ones in the United States.

Reproduction in staging

  • I was unable to find a suitable climb in staging that reproduced the issue, until @musoke suggested I create my own climb. Sure enough, the issue reproduced, which suggests that this occurs with newer climbs.

The fix

  • @daneashman commented in Recently added climb is not allowing me to tick climb (occurring across multiple climbs) #1130 that a data issue may be the culprit. Affected climbs do not have a yds property, while unaffected climbs do.
  • TypeScript hinting in code stated that yds is deprecated
  • The TickForm component is dynamically built when the TickButton component is clicked.
  • The TickButton component is rendered in the ClimbData page, and passes in grade={yds}, which is null on newer climbs
  • I noticed that places where the grade was shown now used the Grade object(?) which had a .toString() method that worked on both Sport/Trad climbs and boulders. I updated the TickButton component to pass in gradesObj.toString() instead of yds
  • Ticking the affected climb on staging now worked.

Copy link

vercel bot commented Oct 31, 2024

@julianlam is attempting to deploy a commit to the openbeta-dev Team on Vercel.

A member of the Team first needs to authorize it.

@clintonlunn
Copy link
Collaborator

@julianlam Would you mind filling in some details in this pr to include reproduction that you've done in the staging env and maybe some screenshots?

@vnugent could you authorize a vercel deployment here?

Copy link

vercel bot commented Oct 31, 2024

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

Name Status Preview Updated (UTC)
open-tacos ✅ Ready (Inspect) Visit Preview Oct 31, 2024 5:59pm

@julianlam
Copy link
Contributor Author

Hi @clintonlunn — done!

It's worth noting here that my use of gradesObj.toString() is not standard. Other places in the codebase have the entire form revamped to use gradesObj entirely, so it could be that this is a band-aid until the TickButton/TickForm are refactored.

However, it is also worth noting that it seems odd to pass in all this extra data to the backend (specifically, name and grade) as those details could be retrieved server-side. I don't have enough expertise in this codebase to ask questions, though, so I'll leave it 😄

@julianlam
Copy link
Contributor Author

@clintonlunn on staging the affected climb is Discovery, addressable via climbs/ad8361ab-94cb-48fe-aa84-48f6a6ba7d12

@clintonlunn
Copy link
Collaborator

Yep, you can see that the yds value is undefined in new climbs but defined in older climbs.

Looks like submitting with an older climb has the grade correctly populated.
image

It threw me a little that the .toString() was not this .toString(). The gradesObj.toString() is pulling out the proper grade from the gradesObj

This looks good to me and unblocks a lot of people! @vnugent

It does seem like the yds property is deprecated and should be removed in a future issue. It's imported into a few components but doesn't seem to be doing anything.

@julianlam
Copy link
Contributor Author

@vnugent May I get a PR review so this can be merged? 🙏

@julianlam julianlam self-assigned this Nov 7, 2024
@clintonlunn clintonlunn self-requested a review November 8, 2024 21:32
Copy link
Collaborator

@clintonlunn clintonlunn left a comment

Choose a reason for hiding this comment

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

Looks good to me. In the future we should see if we can refactor to avoid needing to put a .toString() on this prop.

@clintonlunn clintonlunn merged commit 7b8099f into OpenBeta:develop Nov 8, 2024
3 of 4 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.

Recently added climb is not allowing me to tick climb (occurring across multiple climbs)
3 participants