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

Make BWB + Amz prices lazy load on editions page #8576

Closed
3 tasks
mekarpeles opened this issue Nov 30, 2023 · 10 comments · Fixed by #8824
Closed
3 tasks

Make BWB + Amz prices lazy load on editions page #8576

mekarpeles opened this issue Nov 30, 2023 · 10 comments · Fixed by #8824
Assignees
Labels
Good First Issue Easy issue. Good for newcomers. [managed] Lead: @jimchamp Issues overseen by Jim (Front-end Lead, BookNotes) [managed] Module: Books Page Priority: 2 Important, as time permits. [managed] Theme: Affiliate API Theme: Performance Issues related to UI or Server performance. [managed] Type: Feature Request Issue describes a feature or enhancement we'd like to implement. [managed]

Comments

@mekarpeles
Copy link
Member

mekarpeles commented Nov 30, 2023

Next Steps

  • Decide on approach (e.g. partials + js to fetch prices?) @jimchamp
  • Needs breakdown on which files to edit
  • Needs instructions for how to test -- for now if BWB doesn't return back data, have the partial endpoint return static/fake data. Ignore amz for now.

Design

Patron should see the affiliate/vendors table but show static "tomb stone" grey placeholders until the amz + bwb data becomes available.

Describe the problem that you'd like solved

Justification: Currently BWB + amazon are expensive network calls which result in slow books page load times in sentry

We want to load this data async to speed up the perf.

Proposal & Constraints

Additional context

Stakeholders

@mekarpeles mekarpeles added Type: Feature Request Issue describes a feature or enhancement we'd like to implement. [managed] Good First Issue Easy issue. Good for newcomers. [managed] Module: Books Page Theme: Performance Issues related to UI or Server performance. [managed] Priority: 2 Important, as time permits. [managed] Lead: @jimchamp Issues overseen by Jim (Front-end Lead, BookNotes) [managed] Theme: Affiliate API labels Nov 30, 2023
@nick2432
Copy link

can i work on this?

@tusharv01
Copy link

@mekarpeles Please assign me this issue.

@jimchamp
Copy link
Collaborator

@nick2432 I've assigned you to work on this (sorry for the delay).

Here's some more information about this:

The template for our affiliate links is here. I don't think that this file will need to be modified at all, but it will need to be rendered server-side and returned to the client.

The template for our book page is here. Currently, we store the links as affiliate_links and render this value twice on the book page. This will need to be updated to instead store the HTML for the "tomb stone" placeholder. You can define a new template function that takes isbn, asin, prices, and edition_key as parameters and returns a placeholder element that includes data attributes for each parameter. These values will be used to fetch the HTML for the AffiliateLinks macro.

You'll have to create a new endpoint which will be used to fetch the rendered AffiliateLinks. This can be added to this file for now. You can model the GET handler on this handler, which returns partials for another feature. Since AffiliateLinks.html is macro, the render_template call will need to be replaced with something like macro.macrostore['AffiliateLinks'](doc, opts), where doc is the edition, and opts is a dict containing asin, isbn, and prices. You can get a reference to the edition by calling web.ctx.site.get(edition_key).

The path for the new endpoint can be something like /affiliate-links/partials.

Finally, you'll need to replace the placeholder with the HTML for the affiliate links. Make sure to only fetch the links once per book page, and replace both placeholders with the results.

@spurjhattam2207
Copy link

Please assign me to this issue

@tusharv01
Copy link

@jimchamp Please assign me this issue on a first come basis I am the one whom you assign sir.

@mekarpeles
Copy link
Member Author

@tusharv01 happy to assign you, before starting work, can you please post a comment here describing how you will implement the solution and make sure you tag @jimchamp so he can review your approach. Thank you!

@tusharv01
Copy link

@mekarpeles @jimchamp
Proposed Implementation Approach:

1. Update AffiliateLinks Template:

- Ensure the AffiliateLinks.html template includes necessary placeholders for BWB and Amazon prices.

2. Update Book Page Template:

- Replace the current rendering of 'affiliate_links' with a placeholder element.

- Create a new template function, render_affiliate_links_placeholder, to generate the placeholder.

3. Create a New Endpoint for AffiliateLinks:

- Add a new endpoint '/affiliate-links/partials' to fetch rendered AffiliateLinks.

- Model the GET handler on an existing handler, using the macro.macrostore['AffiliateLinks'](doc, opts) syntax.

4. Replace Placeholders with HTML:

- In the JavaScript part of book_page_template.html, fetch affiliate links using the new endpoint.

- Replace placeholders with the fetched HTML.

@mekarpeles mekarpeles added Lead: @scottbarnes Issues overseen by Scott (Community Imports) and removed Lead: @jimchamp Issues overseen by Jim (Front-end Lead, BookNotes) [managed] labels Jan 29, 2024
@QuantuM410
Copy link
Contributor

@tusharv01 are you still working on this issue?

@tusharv01
Copy link

@tusharv01 are you still working on this issue?

Yes

@mekarpeles mekarpeles added Lead: @jimchamp Issues overseen by Jim (Front-end Lead, BookNotes) [managed] and removed Lead: @scottbarnes Issues overseen by Scott (Community Imports) labels Feb 14, 2024
@jimchamp
Copy link
Collaborator

jimchamp commented Feb 14, 2024

@tusharv01, this issue is rising in priority, and is something that we'd like finished relatively soon. When do you expect to open a PR with your solution?

Edit: Just noticed that you don't have the repo forked, so I'm assuming that you are not working on this. Assigning myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Easy issue. Good for newcomers. [managed] Lead: @jimchamp Issues overseen by Jim (Front-end Lead, BookNotes) [managed] Module: Books Page Priority: 2 Important, as time permits. [managed] Theme: Affiliate API Theme: Performance Issues related to UI or Server performance. [managed] Type: Feature Request Issue describes a feature or enhancement we'd like to implement. [managed]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants