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

DDFBRA-183 - Anonymous and logged in user can try ebook #1555

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

kasperbirch1
Copy link
Contributor

@kasperbirch1 kasperbirch1 commented Nov 20, 2024

Link to issue

https://reload.atlassian.net/browse/DDFBRA-183
https://reload.atlassian.net/browse/DDFBRA-184
danskernesdigitalebibliotek/dpl-design-system#791
danskernesdigitalebibliotek/dpl-cms#1779

Description

This pull request adds a new feature that allows users to try an e-book. It introduces a Reader app that loads teasers based on an identifier. This functionality is compatible with materials classified under the following types:

  • "e-bog"
  • "billedbog (online)"
  • "tegneserie (online)"
  • "årbog (online)"

The Reader app is also designed to handle reserved materials by using an orderId instead of an identifier.

Additional Changes:
- Added a full-screen modal for the reader.

  • Introduced a MaterialSecondaryButton and MaterialSecondaryLink to share behavior between MaterialButtonsFindOnShelf.

Important Notes:
During the implementation, I discovered a bug that requires the use of window.location.reload(); when closing the ReaderModal to enable reopening it. Initially, I suspected this issue was introduced in this pull request. However, further investigation revealed that the same behavior is reproducible with the "FindOnShelf" feature on the live site (https://bibliotek.kk.dk).

A separate ticket has been created to address this issue: https://reload.atlassian.net/browse/DDFHER-140.

The modal error is not related to why the Reader didn’t work. We experienced the same issue in the Go project. The problem seems to occur because Reader loads some scripts during onPageLoad. To address this, we restructured the architecture and made Reader an app instead of a component. This app now resides at /reader?* in the CMS.

Update
The code from danskernesdigitalebibliotek/dpl-react#1562 was so similar that I merged that PR into this one.

This means that it is now possible to “Try eBook” using the Reader component and “Try Audiobook” using the Player.

Here is a matrial that has both ebook and audiobook:
https://varnish.pr-1779.dpl-cms.dplplat01.dpl.reload.dk/work/work-of:870970-basis:138374319?type=lydbog+%28online%29

Test

https://varnish.pr-1779.dpl-cms.dplplat01.dpl.reload.dk/work/work-of:870970-basis:62986115?type=tegneserie+%28online%29

Reader.Teaser.mp4

@kasperbirch1 kasperbirch1 changed the title DDFBRA-183 - anonymous and logged in user can try ebook DDFBRA-183 - Anonymous and logged in user can try ebook Nov 20, 2024
kasperbirch1 added a commit to danskernesdigitalebibliotek/dpl-design-system that referenced this pull request Nov 20, 2024
This is required in the following pull request:
danskernesdigitalebibliotek/dpl-react#1555
@kasperbirch1 kasperbirch1 force-pushed the DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook branch from fa257aa to 55f89e3 Compare November 20, 2024 12:47
@kasperbirch1 kasperbirch1 removed the request for review from LasseStaus November 22, 2024 11:08
The `Reader` component supports both teaser and reserved material.
Use the `identifier` for teaser and `orderId` for reserved.

To load the reader, it is necessary to attach `readerAssets` to the `<head>` dynamically. A function called `appendAsset` has been introduced to manage this process.

Currently, there is a problem where `readerAssets` need to be reloaded each time the reader is initialized. As a temporary workaround, `window.location.reload();` is being used. A better solution will be implemented in the future.
The `Reader` needs to be shown in a full-screen modal.

Note:
It does not work with or need `FocusTrap`.  I have tested a proof-of-concept where it functions as expected. You can navigate pages using the arrow keys and close the modal with the Escape key.
This commit introduces functionality to try e-books.

To support closing the reader via the close/back button, the `closeModal` method was added to the `window` object and invoked using `javascript:window.closeModal('reader-modal-${identifier}')`.
…nal`

Consolidated the "Reader Teaser" logic into `MaterialButtonOnlineExternal` since not all online materials have a teaser.
…ialButtonsFindOnShelf`

These two should follow the same structure to ensure consistency in appearance.
To ensure the `MaterialButtonReaderTeaser` and `MaterialButtonsFindOnShelf` have identical behavior, I created a shared component to eliminate code duplication.
This ensures that the `ReaderModal` is loaded only when necessary, similar to how the `MaterialButtonReaderTeaser` is rendered.
I believe the issue isn't with the `useEffect` inside `Reader`, but rather with the modal logic itself. I have encountered similar issues with the `FindOnShelfModal`, where it doesn't load the second time. A ticket has been created for this issue: https://reload.atlassian.net/browse/DDFHER-140
The addition of `closeModal` to the `window` object is only necessary in the `ReaderModal`, so it is unnecessary to keep it in the generic `Modal` component.
@kasperbirch1 kasperbirch1 force-pushed the DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook branch from d9acb30 to 2847ee8 Compare November 22, 2024 12:39
Since loading the reader scripts requires a page shift, I developed the reader as a separate app with its own route in Drupal.
In the new architecture, `Reader` is now a standalone app.

Additionally I also found that there is no reason to wrap the stories in `Store`
After the `Reader` is no longer an app, the `ReaderModal` is no longer in use and has been removed.

A new component, `MaterialSecondaryLink`, has been added. It follows the same structure as `MaterialSecondaryButton` but is designed for links instead.

I considered reverting the `MaterialSecondaryButton` introduced in an earlier commit, but I believe the new structure is more readable. Additionally, it may be useful for the `Player` app/component in the future.

- Remove unnecessary `isFullScreen`  from `Modal`

With `Reader` now functioning as an  app on its own page, this property is no longer required.
This component is redundant as its functionality can be replaced with `MaterialSecondaryLink`.
@kasperbirch1 kasperbirch1 force-pushed the DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook branch from 2847ee8 to 440e0d6 Compare November 22, 2024 12:49
Implemented the `Player` component, which functions similarly to the `Reader`. Considering moving the `Reader` component to the `components` folder for consistency in folder structure.
This enables the player teaser button for the correct material types: "lydbog (online)", "podcast", "musik (online)", "lydbog (bånd)" that opens in a modal `PlayerModal`.
kasperbirch1 and others added 7 commits November 26, 2024 09:11
These components are very similar and distinct from other components, so I have moved them together into the same folder and created a new story category called "ReaderPlayer" for them.
These will, in the future, not only be used for showing teasers.
The function now accepts a `@materialType`, which can also reference translatable strings such as `ebookText` or `audiobookText`.
This is now handled in `.player` CSS.
…anonymous-and-logged-in-user-can-try-audiobook

DDFBRA-184 - Anonymous and logged in user can try audiobook
This caused issues when passing the URL parameter from Drupal into the `Reader` app.
…ublizon`

Sets the foundation for future changes where all Publizon (ereolen) will be handled internally, and as such, it will no longer be part of `MaterialButtonOnlineExternal`.

For the time being, I have added the new `MaterialButtonOnlinePublizon` below `MaterialButtonOnlineExternal`, but the TODO notes that separate logic will be required to manage the `Reader` / `Player` buttons/links.
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.

5 participants