-
Notifications
You must be signed in to change notification settings - Fork 8
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
kasperbirch1
wants to merge
22
commits into
develop
Choose a base branch
from
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
DDFBRA-183 - Anonymous and logged in user can try ebook #1555
kasperbirch1
wants to merge
22
commits into
develop
from
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
+560
−46
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
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
force-pushed
the
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
branch
from
November 20, 2024 12:47
fa257aa
to
55f89e3
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.
kasperbirch1
force-pushed
the
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
branch
from
November 22, 2024 12:39
d9acb30
to
2847ee8
Compare
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
force-pushed
the
DDFBRA-183-anonymous-and-logged-in-user-can-try-ebook
branch
from
November 22, 2024 12:49
2847ee8
to
440e0d6
Compare
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.
…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
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
Reader
app that loads teasers based on anidentifier
. This functionality is compatible with materials classified under the following types:The
Reader
app is also designed to handle reserved materials by using anorderId
instead of anidentifier
.Additional Changes:
- Added a full-screen modal for the reader.MaterialSecondaryButton
andMaterialSecondaryLink
to share behavior betweenMaterialButtonsFindOnShelf
.Important Notes:
During the implementation, I discovered a bug that requires the use ofwindow.location.reload();
when closing theReaderModal
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