Skip to content

refactor: split AffiliateLinks into AffiliateLinksLoadingIndicator.html for loading indicator#12687

Open
Sanket17052006 wants to merge 2 commits intointernetarchive:masterfrom
Sanket17052006:phase_1-AffiliateLinks-refactor
Open

refactor: split AffiliateLinks into AffiliateLinksLoadingIndicator.html for loading indicator#12687
Sanket17052006 wants to merge 2 commits intointernetarchive:masterfrom
Sanket17052006:phase_1-AffiliateLinks-refactor

Conversation

@Sanket17052006
Copy link
Copy Markdown
Contributor

@Sanket17052006 Sanket17052006 commented May 9, 2026

Part of #12678

This PR is the first phase of #12678 and splits and simplifies AffiliateLinks.html template by moving the loading spineer to AffiliateLinksLoadingIndicator.html

Technical

  • Create AffiliateLinksLoadingIndicator.html for async spinner
  • Simplify AffiliateLinks.html to render buy links only (remove async_load)
  • Update view.html to use new loading indicator macro
  • Remove async_load=False from partials.py call

Testing

  • Go to any book edition page e.g. http://localhost:8080/works/OL45310W/Robinson_Crusoe?edition=key:/books/OL24152177M
  • In the "Buy this book" section verify the "Fetching prices" loading spinner appears on page load.

Screenshot

Stakeholders

@RayBB

Copy link
Copy Markdown

@accesslint accesslint Bot left a comment

Choose a reason for hiding this comment

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

Found 2 issues across 1 rule.

</li>
</ul>
<small>$:_('When you buy books using these links the Internet Archive may earn a <a class="nostyle" href="/help/faq/about#selling">small commission</a>.')</small>
<ul class="buy-options-table">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WCAG 1.3.1: <ul> contains direct text content. Wrap in <li>.

<ul> and <ol> must only contain <li>, <script>, or <template> as direct children.

Details

Screen readers announce list structure ('list with 5 items') based on proper markup. Placing non-<li> elements directly inside <ul> or <ol> breaks this structure. Wrap content in <li> elements, or if you need wrapper divs for styling, restructure your CSS to style the <li> elements directly.

@mekarpeles mekarpeles requested a review from Copilot May 9, 2026 04:26
@mekarpeles
Copy link
Copy Markdown
Member

Thank you for the pull request, @Sanket17052006!

🤖 Copilot has been assigned for an initial review.

The linked issue (#12678) hasn't been triaged yet — triage happens on Mondays and Fridays. There are currently 71 open non-draft PRs ahead of yours.

PR triage checklist (maintainers / Pam)
  • PR description — not empty; explains what the change does and how to verify it
  • References an issue — PR body contains a #NNN reference
    • Linked issue is triaged — has a Priority: * label (not just Needs: Triage)
    • Linked issue is assigned — has at least one assignee
  • Commit history clean — no WIP/fixup/conflict noise; commit messages are meaningful
  • CI passing — no failing check-runs
  • Test cases present — if the change touches substantive logic, test coverage exists or is explained
  • Proof of testing — PR body includes a description of what was tested, a screenshot, or a video

Note

This comment was automatically generated by Pam, Open Library's Project AI Manager, on behalf of @mekarpeles. Pam is designed to provide status visibility, perform basic project management functions and relevant codebase research, and provide actionable feedback so contributors aren't left waiting.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR is phase 1 of #12678, splitting the affiliate-links UI into a dedicated loading-indicator macro and simplifying AffiliateLinks.html so it only renders the resolved “Buy this book” links (with async loading handled by JS + partials).

Changes:

  • Added AffiliateLinksLoadingIndicator.html macro to render the “Fetching prices” spinner with the expected affiliate-links-section data attributes.
  • Updated the edition view template to render the new loading-indicator macro instead of AffiliateLinks(..., async_load=True).
  • Updated AffiliateLinksPartial rendering to call AffiliateLinks without the removed async_load parameter.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
openlibrary/templates/type/edition/view.html Switches initial render to the new loading-indicator macro for affiliate links.
openlibrary/plugins/openlibrary/partials.py Removes async_load=False when rendering macros.AffiliateLinks in the partial.
openlibrary/macros/AffiliateLinksLoadingIndicator.html Introduces a dedicated macro for the async loading spinner wrapper.
openlibrary/macros/AffiliateLinks.html Removes the async_load parameter/branch so the macro always renders resolved links.
Comments suppressed due to low confidence (1)

openlibrary/macros/AffiliateLinks.html:20

  • prices = opts.get('prices') is assigned but never used in this template, which adds noise and can mislead future refactors (especially now that async_load is removed). Please remove the unused variable (or use it explicitly if it still has a purpose).
  $code:
      prices = opts.get('prices')
      isbn = opts.get('isbn', '')
      asin = opts.get('asin', '')

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.

3 participants