www.fgks.org   »   [go: up one dir, main page]
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

Refactor book notes modals #5342

Merged
merged 10 commits into from
Jul 28, 2021
Merged

Refactor book notes modals #5342

merged 10 commits into from
Jul 28, 2021

Conversation

jimchamp
Copy link
Collaborator
@jimchamp jimchamp commented Jun 22, 2021 •
edited by cdrini

Closes #5169

Blocking #5353

Separates book notes and observations into two distinct features, each with a dedicated modal template.

On a book page, the notes modal is opened by clicking the "My book notes" link, located below the star rating. The observations modal can be opened by clicking the link in the stats component.

Technical

  • Existing UserMetadata.html file was replaced with NotesModal.html and ObservationsModal.html
    • Like the original file, these are both macros (we may want to change this)
  • Folder containing JS file was renamed to modals
  • JavaScript file greatly simplified
    • Observations modal no longer populated via JavaScript
  • Toast messages are used to communicate submission status to patrons
    • Toast messages now displayed in a container, allowing multiple messages to be displayed on the screen at once

This PR only covers book page modals. Aggregate notes and observations account page views and other improvements will be handled in future PRs.

Testing

Screenshot

book-notes
Book notes modal

observations
Observations modal (with multiple toast messages)

Stakeholders

@mekarpeles

@jimchamp jimchamp requested a review from mekarpeles June 22, 2021 04:47
@mekarpeles mekarpeles self-assigned this Jun 25, 2021
@mekarpeles mekarpeles added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Jun 25, 2021
@jimchamp jimchamp mentioned this pull request Jun 26, 2021
Merged
@mekarpeles
Copy link
Member

Setting pace (e.g. POSTing to https://testing.openlibrary.org/works/OL3351734W/observations) seems to fail. Either that, or updating the value of pace after set fails.

@mekarpeles mekarpeles added Needs: Feedback A proposed feature or bug resolution needs community feedback prior to forging ahead. [managed] State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] Theme: Book Tags Issues related to community book tags labels Jun 28, 2021
@mekarpeles mekarpeles added this to the Active Sprint milestone Jun 28, 2021
@mekarpeles mekarpeles added Priority: 2 Important, as time permits. [managed] and removed State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] labels Jun 28, 2021
@jimchamp
Copy link
Collaborator Author
jimchamp commented Jun 28, 2021 •
edited

I'm not able to reproduce this. How did you notice that it failed?

@mekarpeles mekarpeles added Priority: 1 Do this week, receiving emails, time sensitive, . [managed] and removed Priority: 2 Important, as time permits. [managed] labels Jul 6, 2021
@cdrini cdrini modified the milestones: Sprint 2021-06, Active Sprint Jul 6, 2021
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Jul 14, 2021
@jimchamp
Copy link
Collaborator Author
jimchamp commented Jul 14, 2021 •
edited

Setting pace (e.g. POSTing to https://testing.openlibrary.org/works/OL3351734W/observations) seems to fail. Either that, or updating the value of pace after set fails.

I see what's wrong now. This branch was not on testing, due to a conflict in js/index.js. This PR does use toast messages (sorry for any confusion about this). Will rebase and attempt to deploy to testing.

Edit: This has been rebased and is on testing.

@jimchamp jimchamp force-pushed the refactor/decouple-booknotes branch from 8af5142 to b7fddae Compare July 14, 2021 19:00
@jimchamp jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Jul 14, 2021
@mekarpeles mekarpeles merged commit a1fb9d7 into internetarchive:master Jul 28, 2021
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mekarpeles mekarpeles Awaiting requested review from mekarpeles

Assignees

@mekarpeles mekarpeles

Labels
Needs: Feedback A proposed feature or bug resolution needs community feedback prior to forging ahead. [managed] Priority: 1 Do this week, receiving emails, time sensitive, . [managed] Theme: Book Tags Issues related to community book tags
Projects
None yet
Milestone
Sprint 2021-07
Development

Successfully merging this pull request may close these issues.

Vulnerability CWE-79 found in patron-metadata/index.js
3 participants
@jimchamp @mekarpeles @cdrini