Reader / Player #1596
Open
Reader / Player #1596
+942
−102
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
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).
While investigating https://reload.atlassian.net/browse/DDFBRA-236, I identified an infinite loop in `MaterialDetailsModal`. This issue appears when the modal is opened via "user/loans" and potentially all places it is used
Since all data from loans comes from Publizon, we don’t have a `workId`, which is required to link to the material. Therefore, I use the `useComplexSearchWithPaginationQuery` to find the work using the ISBN (identifier) and construct the `workId` into a URL, e.g., `/work/work-of:870970-basis:62986115`. Additionally, if it finds the material type, it adds that as a parameter.
This commit ensures that users who already have a digital loan (OrderId) at Publizon will see a button labeled either "Read" or "Listen to @materialType."
### Changes Made:
1. Added `orderId` to `LoanType` and included it in the return of `mapPublizonLoanToLoanType`.
2. Introduced a new hook, `useReaderPlayerButtons`, which accepts `selectedManifestations` and returns an `orderId` if the identifier for the `selectedManifestations` exists in the Publizon loans retrieved from `useLoans`.
### Known Issues:
This is a WIP because an issue with `FocusTrap` still needs to be resolved. Currently, `FocusTrap` is temporarily set to `active={false}` to avoid blocking further testing.
This change was required to avoid removing the `FocusTrap` from the modal, as it states: "Your focus-trap must have at least one container with at least one tabbable node in it at all times." I spent some time attempting to use `createPortal` to manage a single modal in `material.tsx`, but this approach does not work because the modal is only rendered after it is opened. The solution is to include both modals in `material.tsx` and utilize the existing `useReaderPlayerButtons` hook to determine whether the modal should render. This necessitated a small refactor to handle scenarios where there are no `manifestations`. Additionally, I made a minor cleanup, but further improvements are still needed.
Eliminate redundant code, prevent duplicate invocations of `getMaterialTypes`, and consolidate the logic into a single function for determining the type. Use the `type` from `useReaderPlayerButtons` to control when player logic is enabled.
Adamik10
reviewed
Dec 23, 2024
Adamik10
approved these changes
Jan 2, 2025
spaceo
requested changes
Jan 6, 2025
|
|
||
| if (size !== "small") { | ||
| return ( | ||
| <Button |
There was a problem hiding this comment.
is onKeyUp and tabIndex handled in the Button component?
There was a problem hiding this comment.
I think we need to discuss this. I have simply copied the logic from src/components/material/material-buttons/physical/MaterialButtonsFindOnShelf.tsx. Therefore, I would prefer creating a separate pull request, as this is already a very large PR, and I don’t see an easy way to handle it within the Button component since it always has a default "button-like style".
src/components/material/material-buttons/generic/MaterialSecondaryLink.tsx
Outdated
Show resolved
Hide resolved
…anonymous-and-logged-in-user-can-try-ebook DDFBRA-183 - Anonymous and logged in user can try ebook
…logged-in-user-can-go-from-digital-loans-to-material-to-listen-read DDFBRA-236 - Logged in user can go from digital loans to material
…logged-in-user-can-open-their-loaned-e-material WIP: DDFBRA-251 - User can open their loaned e materials
The `isVisible` function in `src/core/utils/featureFlag.ts` manages feature visibility in development and pull request environments. This ensures that new code can be integrated without being exposed to users on production sites. During the interim period before the reader/player logic is fully released, some translatable texts and URLs will be hardcoded. This approach simplifies pull requests and avoids unnecessary complexity.
This was partially removed by misunderstanding in [DDFBRA-49](https://reload.atlassian.net/browse/DDFBRA-49). It has now been reintroduced along with an update to `MaterialAvailabilityTextOnline`, ensuring it checks for all e-materials (Publizon). Here is the commit where it was removed: [b28c225](b28c225)
Replaced `Object.fromEntries` with `reduce` to build the `availabilityTextMap`, making the logic more explicit and readable while preserving the original functionality.
Renamed variables to `shouldShowOnlineAvailability` and `shouldShowPhysicalAvailability` to better reflect their purpose.
Added a condition to return default values when ´manifestations` or 'data?.loans` is `null`, and removed redundant null checks for loans after ensuring its presence earlier.
…ntifier` The updated name provides better clarity, making the function's purpose and usage more apparent to developers. Note: Filename changes will be addressed in a separate commit for git history purposes.
We don’t use the `warn` approach in the codebase. Therefore, this is an intentional error for development purposes until a better solution is implemented.
…in user loans. This hides the link to Erolen when the manifestation is already in the user's loans.
…tensibility - Replaces single function (isVisible) with a featureFlag object - Introduces a features list (name, active), allowing for future expansion - Implements an isActive() method to handle feature toggles more cleanly
Add descriptions for `identifier` and `orderId` and make the fields editable.
kasperbirch1
force-pushed
the
reader-player-feature
branch
from
c2ec163 to
d08cbca
Compare
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-182
https://reload.atlassian.net/browse/DDFBRA-251
https://reload.atlassian.net/browse/DDFBRA-236
https://reload.atlassian.net/browse/DDFBRA-273
Pull requests
#1555
#1565
#1573
Description
This pull request consolidates all features related to Reader/Player functionality, streamlining the integration and development process.
Test
https://varnish.pr-1859.dpl-cms.dplplat01.dpl.reload.dk/
Video
https://reload.atlassian.net/browse/DDFBRA-182
https://reload.atlassian.net/browse/DDFBRA-251
https://reload.atlassian.net/browse/DDFBRA-236
Min.film.1.mp4
https://reload.atlassian.net/browse/DDFBRA-273
Min.film.2.mp4