DDFBRA-183 - Anonymous and logged in user can try ebook #1555
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kasperbirch1
changed the title
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
force-pushed
the
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
branch
from
fa257aa to
55f89e3
Compare
kasperbirch1
force-pushed
the
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
branch
2 times, most recently
from
2847ee8 to
440e0d6
Compare
Adamik10
requested changes
Dec 2, 2024
There was a problem hiding this comment.
Beautiful work :) I have a couple comments.
Also I'm not sure if it's just happening to me, but if I stay on this page (player on the demo site) for long enough (3-10 mins) it throws an error (see picture) -
src/components/material/material-buttons/generic/MaterialSecondaryButton.tsx
Show resolved
Hide resolved
src/components/material/material-buttons/generic/MaterialSecondaryButton.tsx
Show resolved
Hide resolved
src/components/material/material-buttons/online/MaterialButtonOnlineExternal.tsx
Outdated
Show resolved
Hide resolved
src/components/material/material-buttons/online/MaterialButtonOnlinePublizon.tsx
Outdated
Show resolved
Hide resolved
src/components/material/material-buttons/online/MaterialButtonOnlinePublizon.tsx
Outdated
Show resolved
Hide resolved
src/components/material/material-buttons/generic/MaterialSecondaryButton.tsx
Show resolved
Hide resolved
kasperbirch1
force-pushed
the
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
branch
from
665996b to
e8a38e8
Compare
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.
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`.
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`.
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.
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.
This is no longer used as the `Reader` is now on its own app (page).
kasperbirch1
force-pushed
the
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
branch
from
d01a598 to
bd60ac0
Compare
kasperbirch1
deleted the
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
branch
kasperbirch1
added a commit
that referenced
this pull request
Dec 17, 2024
…anonymous-and-logged-in-user-can-try-ebook DDFBRA-183 - Anonymous and logged in user can try ebook
kasperbirch1
added a commit
that referenced
this pull request
Jan 7, 2025
…anonymous-and-logged-in-user-can-try-ebook DDFBRA-183 - Anonymous and logged in user can try ebook
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Readerapp that loads teasers based on anidentifier. This functionality is compatible with materials classified under the following types:The
Readerapp is also designed to handle reserved materials by using anorderIdinstead of anidentifier.Additional Changes:
- Added a full-screen modal for the reader.MaterialSecondaryButtonandMaterialSecondaryLinkto share behavior betweenMaterialButtonsFindOnShelf.Important Notes:
During the implementation, I discovered a bug that requires the use ofwindow.location.reload();when closing theReaderModalto 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