fix: disable 'Want to Read' button until JS loads #12139
Conversation
bhardwajparth51
changed the title
bhardwajparth51
force-pushed
the
fix/want-to-read-js-fallback
branch
from
a7a83ff to
8fd5306
Compare
|
decided to keep the button fully opaque ( |
bhardwajparth51
force-pushed
the
fix/want-to-read-js-fallback
branch
from
8fd5306 to
1a53226
Compare
…ds to prevent JSON dead end
bhardwajparth51
force-pushed
the
fix/want-to-read-js-fallback
branch
from
4e0ca39 to
bdff092
Compare
github-actions
Bot
added
the
Needs: Response
|
@jimchamp I noticed #12140 by @mekarpeles addresses the same issue. Happy to close this in favour of that PR, or contribute any differences if useful. |
| if (this.primaryButton) { | ||
| this.primaryButton.removeAttribute('disabled') | ||
| } |
There was a problem hiding this comment.
This needs to be moved outside of the if(!this.isDropperDisabled) block. Otherwise, clicking the "Add to List" button does not redirect to the login page.
jimchamp
added
Needs: Submitter Input
|
@bhardwajparth51 were you able to take this one over the finish line? |
Moves `removeAttribute` outside the if (!this.isDropperDisabled) guard as Jim suggested — this ensures the button is re-enabled for both logged-in and logged-out users. When isDropperDisabled is true (e.g. user not logged in), the button currently stays permanently disabled, so clicks never fire and the form never submits to the login redirect.
github-actions
Bot
removed
the
Needs: Submitter Input
|
Worth noting that this will break the experience for those without js enabled (as the button will be permanently disabled). Let's just note this for @seabelis in case this becomes an issue for patrons. |
There was a problem hiding this comment.
Pull request overview
Disables the “Want to Read” (reading log primary action) button during initial page load to prevent pre-JS clicks from submitting the form and landing users on a JSON response page.
Changes:
- Renders the primary reading-log CTA button as disabled in the initial server-side template output.
- Adds CSS to prevent the pointer cursor (and avoid disabled-state flicker) on the disabled primary button.
- Updates
ReadingLogForms.initialize()to re-enable the primary button once the JS component initializes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
openlibrary/templates/my_books/primary_action.html |
Disables the primary reading-log submit button in initial HTML. |
static/css/components/generic-dropper.css |
Adds disabled-state cursor/opacity override for the primary button. |
openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper/ReadingLogForms.js |
Re-enables the primary button at component initialization time. |
| * If dropper is disabled, no event listeners will be added. | ||
| */ | ||
| initialize() { | ||
| if (this.primaryButton) { |
There was a problem hiding this comment.
initialize() removes disabled from primaryButton even when the dropper is marked disabled (generic-dropper--disabled). This conflicts with the method docstring (“If dropper is disabled, no event listeners will be added”) and can re-enable a control that the UI intends to keep disabled. Consider only re-enabling the button when !this.isDropperDisabled (or otherwise gating it on authentication/eligibility).
| if (this.primaryButton) { | |
| if (this.primaryButton && !this.isDropperDisabled) { |
There was a problem hiding this comment.
The this.isDropperDisabled flag is only set when the patron is logged out. When true, the dropper cannot be opened. It has nothing to do with the primary button's disabled attribute.
There was a problem hiding this comment.
I'm pretty sure that the suggested code change puts us in the same position as we are now -- clicks on the "Add to List" button will not redirect to the login page.
| * If dropper is disabled, no event listeners will be added. | ||
| */ | ||
| initialize() { | ||
| if (this.primaryButton) { |
There was a problem hiding this comment.
New behavior is introduced where the primary reading-log button starts disabled in HTML and is enabled during initialize(). There are existing Jest unit tests for the my-books UI, but none cover ReadingLogForms.initialize(); adding a unit test that asserts the button is enabled after initialization (and stays disabled for disabled droppers, if applicable) would help prevent regressions.
| if (this.primaryButton) { | |
| if (this.primaryButton && !this.isDropperDisabled) { |
Co-authored-by: Copilot <[email protected]>
Description
Closes #12042
This PR fixes the issue where clicking the "Want to Read" button before the page is fully loaded would lead to a broken JSON response. I've disabled the button in the initial HTML so users can't click it mid-load, and it gets re-enabled automatically once the JavaScript is ready to handle the click.
Technical
To get this working correctly, I had to touch a few different places:
disabledattribute to the main button inopenlibrary/templates/my_books/primary_action.htmlstatic/css/components/generic-dropper.cssto make sure the mouse cursor stays as a default arrow (rather than a pointer) while the button is disabled. I used a high-specificity selector to make sure it correctly overrides the global button styles.3.Updated
ReadingLogForms.jsstrip thedisabledattribute once the component initializes.Testing
To verify the fix, you can simulate a slow load (or just disable JS) and go to any book page.
Screenshots
simplescreenrecorder-2026-03-19_22.43.43.online-video-cutter.com.mp4
Screencast_from_19-03-26_10_15_32_PM_IST_.1.-compressed-file-converter-online.com.mp4
Stakeholders
@jimchamp