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.

Sign up for GitHub

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

Jump to bottom

Guides: Receive from new api feature #1800

Merged
merged 1 commit into from
Jul 6, 2023
Merged

Guides: Receive from new api feature #1800

merged 1 commit into from
Jul 6, 2023

Conversation

jarstelfox
Copy link
Contributor

@jarstelfox jarstelfox commented Jul 3, 2023 •
edited
Loading

After https://github.com/iFixit/ifixit/pull/48619, there is now a param to the API which will return the guides in the same shape as the /guides/id API.

This saves us multiple server-side requests.

This pull updates the code to use the flag and drops multiple requests in the process.

QA

  • Ensure embed guides are still shown at the bottom of a section

Connects: https://github.com/iFixit/ifixit/issues/48100

@jarstelfox
Guides: Recieve from new api feature
f744fac 
We added a param to the api which will return the guides in the same
shape as the /guides/id api.

This saves us multiple serverside requests.
@vercel
Copy link

vercel bot commented Jul 3, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
react-commerce ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 3, 2023 8:01pm
react-commerce-prod ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 3, 2023 8:01pm

@jarstelfox jarstelfox added the external_block QA is not testing this (yet?) label Jul 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2023

📦 Next.js Bundle Analysis for @ifixit/commerce-frontend

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@jarstelfox jarstelfox removed the external_block QA is not testing this (yet?) label Jul 5, 2023
@erinemay
Copy link
Contributor

erinemay commented Jul 5, 2023 •
edited
Loading

QA 👍 guide cards are showing up on this branch without error.

The guide cards look identical.

@jarstelfox, I found an issue with https://github.com/iFixit/ifixit/pull/48619 while testing this. Making a new issue.

Guides whose master langid is not English are showing up in the master langid.
https://www.cominor.com/Guide/MacBook+Pro+13-Inch+Unibody+Mid+2010+Key+Cap+Removal/132050
https://www.cominor.com/api/2.0/Troubleshooting/MacBook%20Keyboard%20Not%20Working?fullGuides=1
https://react-commerce-git-troubleshooting-use-guide-flag-ifixit.vercel.app/Vulcan/MacBook%20Keyboard%20Not%20Working#Section_Debris_in_Keys_or_The_Butterfly_Keyboard_Effect
image

EDIT: https://github.com/iFixit/ifixit/issues/48637

hcutrone
hcutrone approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@hcutrone hcutrone left a comment

Choose a reason for hiding this comment

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

CR 🌯

@jarstelfox
Copy link
Contributor Author

Guides whose master langid is not English are showing up in the master langid.

Pull open: https://github.com/iFixit/ifixit/pull/48639 🚀

@jarstelfox jarstelfox merged commit 308fc4c into main Jul 6, 2023
13 checks passed
@jarstelfox jarstelfox deleted the troubleshooting-use--guide-flag branch July 6, 2023 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hcutrone hcutrone hcutrone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jarstelfox @erinemay @hcutrone