Added a QR icon in Share Modal that opens to QR code on click #9233

merwhite11 · 2024-05-07T21:02:53Z

Closes #452

Feature: Adds a QR code icon to the share modal on books page. On icon click, a new tab opens with the QR code image.
Refactor: Changed styling of Share Modal so that for small -> tablet sized screens, the share modal icons appear larger and in two rows.

Technical

Installed qrcode dependency in requirements.txt
Imported qrcode and io in api.py for use in create_qrcode handler.

Testing

docker compose run --rm home make test : passing with warnings
docker compose run --rm home pytest openlibrary/plugins/importapi/tests/test_import_validator.py: passing with warnings
docker compose run --rm home npm run lint: passing with deprecation warnings

Screenshot

I can change the size of the icons if these are too large. I can also change the breakpoint for when to switch from large 2-row icons, to smaller single row icons. Currently I have the icons in 2 rows for everything tablet and smaller.

Stakeholders

@jimchamp
Thank you for the help!

codecov-commenter · 2024-05-08T19:28:26Z

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 16.11%. Comparing base (db9ead2) to head (936d6e1).
Report is 141 commits behind head on master.

Files	Patch %	Lines
openlibrary/plugins/openlibrary/js/modals/index.js	0.00%	27 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9233      +/-   ##
==========================================
+ Coverage   15.89%   16.11%   +0.22%     
==========================================
  Files          90       91       +1     
  Lines        4732     4785      +53     
  Branches      824      829       +5     
==========================================
+ Hits          752      771      +19     
- Misses       3468     3498      +30     
- Partials      512      516       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

merwhite11 · 2024-05-22T21:37:57Z

@jimchamp Could you review this when you get the chance? Thanks !

jimchamp

Thanks @merwhite11, and sorry for the massive delay!

I've added a number of suggested changes, but they should all be pretty straight-forward. There will be more details in the following comments, but generally, the following should be addressed:

Missing opening div tag has caused the page's layout to shift.
Generated QR code always links to http://localhost:8080/qrcode
The new addQRCodeListener JS function isn't needed if an href is set on the QR code link.
The displayDynamicModal and addShareModalClickListeners functions are very similar to the existing displayModal and addClickListeners functions. I think that displayModal can be modified to handle the case where screen width is less than maxWidth (if such a change is needed). If that's true, the two new functions can be removed.

By the way, I'm uploading QR codes to this site in order to test that the encoded URL is correct.

jimchamp · 2024-05-30T19:45:56Z

openlibrary/macros/ShareModal.html

@@ -14,7 +14,6 @@
  <div class="hidden">
    <div id="social-modal-content" class="floaterAdd">
      <div class="content">
-        <div class="floaterHead right-justify">


It looks like this change is causing the main content of the page to appear below the #contentBody div:

It's also causing the modal's X close affordance to appear on the upper-left side (instead of the upper-right):

At first glance, everything looks fine when this opening div tag is added back to the template.

Oh I see what I did there. Fixed the spacing.

openlibrary/plugins/openlibrary/api.py

openlibrary/plugins/openlibrary/js/modals/index.js

jimchamp · 2024-05-30T21:11:03Z

openlibrary/plugins/openlibrary/js/modals/index.js

+function displayDynamicModal(content, maxWidth) {
+    const maxWidthNum = parseInt(maxWidth.replace('px', ''));
+    const modalId = `#${content.id}`;
+    const context = content.dataset['context'] ? JSON.parse(content.dataset['context']) : null;
+    const reloadId = context ? context.reloadId : null;
+
+    function openModal() {
+        $.colorbox({
+            inline: true,
+            opacity: '0.5',
+            href: modalId,
+            width: '100%',
+            maxWidth: window.innerWidth > maxWidthNum ? maxWidth : '100%',
+            onClosed: function () {
+                if (reloadId) {
+                    $(`#${reloadId}`).trigger('contentReload');
+                }
+            },
+        });
+    }
+
+    openModal();
+
+    $(window).on('resize', function () {
+        $.colorbox.remove();
+        openModal();
+    });
+}


It looks like this is exactly the same as the displayModal function, except for the change that sets maxWidth to 100% if the screen's width is less than the value that is passed.

I think that we can avoid this repeated code by modifying displayModal to include the changes that you added to displayDynamicModal.

Actually, this change may not be needed at all. While testing, I replaced your displayDynamicModal call with a displayModal call, and everything looked the same in both mobile and desktop views (tested in Firefox and Chrome).

Yes testing this myself and realizing displayDynamicModal is not needed. I can use the pre-existing addClickListeners and addShareModalButtonListeners. I was trying to make it resize appropriately with the dynamic version (eg. if you open in mobile and then switch to desktop, the modal gets out of proportion), but the dynamic isn't handling that correctly either. Is that something we need to take into account for modal testing?

I doubt that this will affect many people, but thanks for pointing it out!

static/css/components/shareLinks.less

jimchamp · 2024-05-30T21:20:20Z

openlibrary/plugins/openlibrary/js/modals/index.js

+function addShareModalClickListeners($modalLinks, maxWidth) {
+    $modalLinks.each(function (_i, modalLinkElement) {
+        $(modalLinkElement).on('click', function () {
+            // Get context, which is attached to the modal content
+            const content = getModalContent($(this));
+            displayDynamicModal(content, maxWidth);
+        });
+    });
+}
+


I think that we can DRY this code by modifying displayModal, and using addClickListeners to add the listeners to the share modal.

openlibrary/macros/ShareModal.html

…nsive

for more information, see https://pre-commit.ci

jimchamp

This seems to be working well --- thanks so much!

merwhite11 force-pushed the 452/feature/add-qr-to-share-modal branch from 3416158 to 2e9a182 Compare May 8, 2024 19:24

merwhite11 force-pushed the 452/feature/add-qr-to-share-modal branch 2 times, most recently from 43e71a7 to 52650ba Compare May 8, 2024 19:35

mekarpeles assigned jimchamp May 9, 2024

merwhite11 force-pushed the 452/feature/add-qr-to-share-modal branch 2 times, most recently from fc03faf to 6939cc6 Compare May 22, 2024 21:07

jimchamp requested changes May 30, 2024

View reviewed changes

jimchamp added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] labels May 30, 2024

merwhite11 and others added 13 commits May 31, 2024 10:01

Lots of experiments for breaking row into 2 lines

582bc07

Experimenting with calling function

4a1ddbc

QR code generating and opening in new tab, added icon png

6d15428

Attempt at making rows not working

936ce1b

Partially working rows

c699f81

Sharemodal splitting into two rows but box height and width not respo…

a78c7e1

…nsive

Created variable for dynamic display function

6463784

Restoring changes after dropping commit in rebase

7df2911

Reverting unintentional style changes to index.js

069e0f5

Fixed lint errors, changed fetch to ajax GET

25081d7

Added newline at end of api.py to pass ruff

fa454fe

[pre-commit.ci] auto fixes from pre-commit.com hooks

29cbd98

for more information, see https://pre-commit.ci

Added back floaterHead div

7846873

merwhite11 force-pushed the 452/feature/add-qr-to-share-modal branch from 05459fa to 7846873 Compare May 31, 2024 17:34

merwhite11 added 2 commits May 31, 2024 12:30

Code Review changes

126f62e

Using displayModal, took out dynamicDisplayModal

e175c61

jimchamp approved these changes May 31, 2024

View reviewed changes

jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label May 31, 2024

jimchamp merged commit 39c7a62 into internetarchive:master May 31, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a QR icon in Share Modal that opens to QR code on click #9233

Added a QR icon in Share Modal that opens to QR code on click #9233

merwhite11 commented May 7, 2024 •

edited

codecov-commenter commented May 8, 2024

merwhite11 commented May 22, 2024

jimchamp left a comment

jimchamp May 30, 2024

merwhite11 May 31, 2024

jimchamp May 30, 2024

jimchamp May 30, 2024

merwhite11 May 31, 2024

jimchamp May 31, 2024

jimchamp May 30, 2024

jimchamp left a comment

Added a QR icon in Share Modal that opens to QR code on click #9233

Added a QR icon in Share Modal that opens to QR code on click #9233

Conversation

merwhite11 commented May 7, 2024 • edited

Technical

Testing

Screenshot

Stakeholders

codecov-commenter commented May 8, 2024

Codecov Report

merwhite11 commented May 22, 2024

jimchamp left a comment

Choose a reason for hiding this comment

jimchamp May 30, 2024

Choose a reason for hiding this comment

merwhite11 May 31, 2024

Choose a reason for hiding this comment

jimchamp May 30, 2024

Choose a reason for hiding this comment

jimchamp May 30, 2024

Choose a reason for hiding this comment

merwhite11 May 31, 2024

Choose a reason for hiding this comment

jimchamp May 31, 2024

Choose a reason for hiding this comment

jimchamp May 30, 2024

Choose a reason for hiding this comment

jimchamp left a comment

Choose a reason for hiding this comment

merwhite11 commented May 7, 2024 •

edited