Navigation between pages and books with history #347
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
.storybook/main.tsOops! Something went wrong! :( ESLint: 9.13.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThe changes in this pull request encompass updates to configuration files, CSS styles, and components related to the Bloom Player project. Key modifications include enhancements to Storybook configuration, the introduction of new CSS files for styling multilingual books, and updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
🧹 Outside diff range and nitpick comments (51)
public/testBooks/multibook-target1/Traditional-XMatter.css (3)
1-9: LGTM! Consider documenting the hidden "Traditional" text usage.The pseudo-element implementation is clean and follows best practices. The hidden "Traditional" text might be used in specific contexts - consider adding a comment explaining its purpose.
218-226: Consider using modern CSS gap propertyThe flex gaps could be simplified using the modern
gapproperty instead of dedicated gap elements.Instead of using elements for gaps:
- .titlePage .smallFlexGap { - flex: 0 1 0.5em; - } - .titlePage .largeFlexGap { - flex: 0 4 3em; - } - .titlePage .fillPageFlexGap { - flex: 1 4 3em; - }Consider using:
.titlePage .marginBox { display: flex; flex-direction: column; gap: var(--title-page-gap, 1em); }
1-262: Consider adopting CSS custom properties for better maintainabilityThe stylesheet implements a comprehensive layout system, but could benefit from better variable usage:
- Define a set of CSS custom properties for commonly used values (margins, paddings, colors)
- Create a consistent naming convention for variables
- Consider breaking the file into logical sections with comments
This would improve maintainability and make the styling system more flexible for different book formats.
public/testBooks/multibook-index/branding.css (2)
1-9: Update documentation to reflect CSS instead of LESSThe comments reference
.lessfiles (bloom-xmatter-commons.lessandbranding-base.less), but this is a CSS file. This might cause confusion for developers.Consider updating the documentation to reference the correct file types or clarify if there's a LESS to CSS compilation step in the build process.
10-24: Consider adding fallbacks for better browser compatibilityThe styles use modern CSS features like flexbox and
object-fit. While these are well-supported, consider adding fallbacks for older browsers.[data-book*="branding"] { display: flex; + display: -webkit-flex; /* Safari */ flex-direction: column; + -webkit-flex-direction: column; /* Safari */ } [data-book*="branding"] img { margin-left: auto; margin-right: auto; width: auto; max-width: 100%; align-self: center; + -webkit-align-self: center; /* Safari */ object-fit: contain; + font-family: "object-fit: contain"; /* object-fit polyfill */ }public/testBooks/multibook-target1/appearance.css (3)
1-118: Consider improving variable naming and fixing comment formatting.While the implementation is thorough and well-documented, there are a few suggestions for improvement:
- Consider shortening
--page-and-marginBox-are-same-color-multiplicandto something more concise like--page-marginBox-color-match.- The comment at line 103 is missing a closing parenthesis.
- The comment at lines 115-116 is incorrectly split across lines.
- --page-and-marginBox-are-same-color-multiplicand: 1; + --page-marginBox-color-match: 1; - /* it is not transparent because at edit time we set the parent (the translationGroup) color to show padding + /* it is not transparent because at edit time we set the parent (the translationGroup) color to show padding) */ - /* In some themes, at edit- - time, the format button lands under the page number. See BL-13206. The name is is intentionally vague about direction and what we do with it. */ + /* In some themes, at edit-time, the format button lands under the page number. + See BL-13206. The name is intentionally vague about direction and what we do with it. */
173-178: Document the necessity of!importantusage.While the use of
!importantmight be necessary here, it's good practice to document why it's required to prevent future maintainers from trying to remove it.- --pageNumber-extra-height: 8mm !important; + /* !important needed to override theme-specific values that might be applied later */ + --pageNumber-extra-height: 8mm !important;
220-229: Consider using a semantic color variable.For better maintainability and theme consistency, consider using a semantic color variable for the cover background.
+ /* At the top of the file */ + :root { + --color-cover-background: #7bdcb5; + } - --cover-background-color: #7bdcb5; + --cover-background-color: var(--color-cover-background);public/testBooks/multibook-target1/previewMode.css (3)
16-24: Consider using a more maintainable selector patternThe hard-coded ID
#ddd9b811-63d6-4d51-baf8-0a5fe88cd73dmakes the styles brittle and difficult to maintain. Consider using a more semantic class name or data attribute to target device-specific layouts.-.preview:not([data-l1]) .bloom-frontMatter.Device16x9Landscape#ddd9b811-63d6-4d51-baf8-0a5fe88cd73d .bookTitle { +.preview:not([data-l1]) .bloom-frontMatter.Device16x9Landscape[data-layout="cover"] .bookTitle { min-height: 10%; }
51-54: Decouple styling from JavaScript functionalityThe comment indicates a tight coupling between CSS and JavaScript for readonly state management. Consider using a dedicated class or attribute for this purpose instead.
-textarea { - /*we have javascript function which notices this border and makes it read-only - (which isn't a style, so we can't set it ourselves*/ - border: 1px solid transparent; -} +textarea { + border: 1px solid transparent; +} +textarea[data-readonly="true"] { + pointer-events: none; + background-color: #f5f5f5; +}
105-113: Review z-index stacking contextThe
z-index: 10000seems arbitrarily high and could cause issues with other overlapping elements. Consider using a lower value and documenting the stacking context hierarchy..preview.template .numberedPage .pageLabel, .bloomPlayer-page.template .numberedPage .pageLabel { position: absolute; - z-index: 10000; + z-index: 100; /* Ensure this is higher than other page elements but not arbitrarily high */ background-color: #d65649; color: white; padding: 5px; display: block; }public/testBooks/multibook-index/index.htm (1)
16-32: Modernize CSS implementation
- The CDATA section is unnecessary in HTML5
- Consider moving styles to an external stylesheet for better maintainability
- <style type="text/css" title="userModifiedStyles"> - /*<![CDATA[*/ + <style title="userModifiedStyles"> .BigWords-style { font-size: 45pt !important; text-align: center !important; } - /*]]>*/ </style>public/testBooks/multibook-target1/index.htm (2)
6-13: Remove unnecessary closing tags for meta elements.Meta tags are self-closing in HTML5 and don't need separate closing tags.
- <meta charset="UTF-8"> - </meta> - <meta name="Generator" content="Bloom Version 6.2.0 (apparent build date: 27-Nov-2024)"> - </meta> - <meta name="BloomFormatVersion" content="2.1"> - </meta> - <meta name="pageTemplateSource" content="Basic Book"> - </meta> + <meta charset="UTF-8"> + <meta name="Generator" content="Bloom Version 6.2.0 (apparent build date: 27-Nov-2024)"> + <meta name="BloomFormatVersion" content="2.1"> + <meta name="pageTemplateSource" content="Basic Book">
39-56: Enhance security for external resources.Consider the following security improvements:
- Add integrity hashes for CSS files to prevent tampering
- Implement Content Security Policy (CSP) headers
- Validate user input in contenteditable elements
Example of adding integrity hash:
- <link rel="stylesheet" href="basePage.css" type="text/css"> + <link rel="stylesheet" href="basePage.css" type="text/css" integrity="sha384-[hash]" crossorigin="anonymous">public/testBooks/multibook-target1/branding.css (4)
1-9: Consider enhancing the documentation clarityWhile the documentation effectively explains the separation of concerns, consider:
- Using more specific version number format (e.g., 6.0.0)
- Adding absolute paths to referenced files (bloom-xmatter-commons.less, branding-base.less)
- compatibleWithAppearanceVersion: 6.0; + compatibleWithAppearanceVersion: "6.0.0";
10-21: Consider adding vendor prefixes for better browser compatibilityThe flexbox and object-fit properties might need vendor prefixes for older browsers.
[data-book*="branding"] { display: flex; + display: -webkit-flex; + display: -ms-flexbox; flex-direction: column; + -webkit-flex-direction: column; + -ms-flex-direction: column; } [data-book*="branding"] img { margin-left: auto; margin-right: auto; width: auto; max-width: 100%; align-self: center; + -webkit-align-self: center; + -ms-flex-item-align: center; object-fit: contain; + -o-object-fit: contain; }
22-24: Add documentation for the outside back cover ruleConsider adding a comment explaining why this element needs 100% width.
+/* Ensures the back cover branding spans the full width of its container */ [data-book="outside-back-cover-branding-bottom-html"] { width: 100%; }
25-54: Consider optimizing orientation-specific rulesThe current implementation has a lot of repetition. Consider using CSS custom properties or a more maintainable approach.
+/* Define reusable classes for orientations */ +.Portrait { + &[class*="Size6x9"], &[class*="USComic"], + &[class*="Device16x9"], &[class*="QuarterLetter"], + &[class*="HalfLetter"], &[class*="Letter"], + &[class*="Legal"], &[class*="A3"], + &[class*="A4"], &[class*="A5"], + &[class*="B5"], &[class*="A6"] { + .portraitOnly { + display: unset; + } + } +}.storybook/main.ts (1)
23-25: Consider externalizing the book ID to name mappingHardcoding the
idToBookNamemapping within the code may reduce maintainability as the list grows or changes. Consider externalizing this mapping to a configuration file or environment variable to improve flexibility and ease of updates.public/testBooks/multibook-target1/basePage.css (2)
1-1304: Consider modularizing the CSS for better maintainabilityThe CSS file contains over 1300 lines of code. To improve maintainability and readability, consider splitting the CSS into modular components or using a CSS preprocessor like Sass or Less to organize styles logically.
689-694: Remove unnecessary comment/*+init {*/At line 689, there is an incomplete comment
/*+init {*/. Please remove or correct this comment to improve code clarity.src/bloom-player-core.tsx (2)
1898-1898: Simplify null checks using optional chainingYou can use optional chaining to simplify the condition:
if (result?.data) { combinedStyle += result.data; }🧰 Tools
🪛 Biome (1.9.4)
[error] 1898-1898: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
1698-1700: Handle rejected promises to prevent unhandled rejectionsThe
axios.get(href)call does not have a catch clause. If the request fails, it may lead to an unhandled promise rejection. Consider adding a catch clause to handle errors gracefully.Apply this diff to handle errors:
axios.get(href).then((result) => { // Existing code }) + .catch((error) => { + // Handle error or log it if necessary + console.error(`Failed to load fonts.css from ${href}:`, error); + });src/stories/BloomPlayerIframe.tsx (1)
16-19: Consider adding error boundaries and loading statesThe iframe implementation could benefit from error handling and loading states:
- Add error boundary for iframe loading failures
- Add loading indicator while iframe is initializing
Consider adding:
return ( + <ErrorBoundary fallback={<div>Failed to load Bloom Player</div>}> <iframe src={`/bloomplayer.htm?url=${encodeURIComponent(bookUrl)}&start-page=${bookPageIndex}${backButtonParam}`} style={{ width: "100%", height: "500px" }} + onLoad={() => setLoading(false)} + onError={(e) => console.error("Failed to load Bloom Player:", e)} /> + </ErrorBoundary> );src/stories/multiBook.stories.tsx (1)
34-35: Clean up commented code and standardize story names
- Remove the commented out storyName
- Use consistent quote style in story names
Apply this change:
- // doesn't work storyName: "Use `bookPageIndex` to specify the starting page", }; -UsePageIndex.storyName = - "Use `bookPageIndex` to specify the starting page of the first book"; +UsePageIndex.storyName = "Use bookPageIndex to specify the starting page of the first book";Also applies to: 36-37
README-advanced.md (1)
28-28: Improve documentation clarity and grammarConsider these style improvements:
- Replace "so as to" with "to"
- Replace "a variety of" with "many" or "various"
- Fix "both ... as well as" construction
Apply these changes:
-`Bloom-player` is designed to be published using `npm` so as to be readily available to a variety of clients. The published version includes both `dist/bloomPlayer-HASH.js`, as well as some mp3 files used by activities. +`Bloom-player` is designed to be published using `npm` to be readily available to various clients. The published version includes both `dist/bloomPlayer-HASH.js` and mp3 files used by activities.🧰 Tools
🪛 LanguageTool
[style] ~28-~28: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...is designed to be published usingnpmso as to be readily available to a variety of cl...(SO_AS_TO)
[style] ~28-~28: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...npmso as to be readily available to a variety of clients. The published version includes...(A_VARIETY_OF)
[grammar] ~28-~28: Probable usage error. Use “and” after ‘both’.
Context: ...cludes bothdist/bloomPlayer-HASH.js, as well as some mp3 files used by activities.W...
(BOTH_AS_WELL_AS)
src/navigation.ts (1)
1-7: Consider using a more robust data structure for historyThe current implementation uses a simple array for history management. Consider using a class or more structured approach that could:
- Enforce maximum history size to prevent memory issues
- Add methods for history manipulation
- Include timestamps for potential time-based features
-const jumpHistory: { bookId: string; pageId: string }[] = []; +class NavigationHistory { + private readonly maxSize = 100; + private history: Array<{ + bookId: string; + pageId: string; + timestamp: number; + }> = []; + + push(bookId: string, pageId: string): void { + if (this.history.length >= this.maxSize) { + this.history.shift(); + } + this.history.push({ bookId, pageId, timestamp: Date.now() }); + } + + pop() { + return this.history.pop(); + } + + canGoBack(): boolean { + return this.history.length > 0; + } +}src/stories/navigation.stories.tsx (1)
12-23: Consider adding more configuration optionsThe story configuration could benefit from additional controls:
- Initial history state
- Network delay simulation
- Error injection
const meta: Meta<typeof BloomPlayerIframe> = { title: "Automated/Navigation", component: BloomPlayerIframe, argTypes: { showBackButton: { control: "boolean", description: "Show back button in the player that will send a message to the host when the local history is empty and the user clicks it.", defaultValue: false, }, + initialHistory: { + control: "object", + description: "Initial navigation history state", + defaultValue: [], + }, + networkDelay: { + control: "number", + description: "Simulate network delay in milliseconds", + defaultValue: 0, + }, }, };src/navigation.test.ts (2)
16-26: Consider adding edge cases for canGoBack tests.While the basic cases are covered, consider adding tests for:
- Multiple navigation steps
- Edge cases with rapid back navigation
138-152: Consider enhancing the createClickEvent helper.The helper function could be more robust by:
- Adding TypeScript types for the event object
- Supporting additional event properties that might be needed in future tests
-function createClickEvent(href: string) { +interface MockClickEvent { + target: { + attributes: { + href: { nodeValue: string } + }, + closest: () => any + }, + preventDefault: () => void, + stopPropagation: () => void +} + +function createClickEvent(href: string): MockClickEvent {README.md (1)
57-67: Fix grammatical issues in the showBackButton section.There are a few grammatical issues to address:
-If true, displays a button in the upper left corner of the app bar. When clicked, bloom-player will post a message "backButtonClicked" that the your host can catch and respond to. +If true, displays a button in the upper-left corner of the app bar. When clicked, bloom-player will post a message "backButtonClicked" that your host can catch and respond to. -However if bloom-player is the top level window +However, if bloom-player is the top-level window🧰 Tools
🪛 LanguageTool
[uncategorized] ~57-~57: Consider adding a hyphen.
Context: ...tton If true, displays a button in the upper left corner of the app bar. When clicked, bl...(TOP_LEFT_CORNER)
[grammar] ~57-~57: A determiner cannot be combined with a possessive pronoun. Did you mean simply “the” or “your”?
Context: ...post a message "backButtonClicked" that the your host can catch and respond to. Example...(A_MY)
[uncategorized] ~64-~64: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...mally this button will be a left arrow. However if bloom-player is the top level window...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~64-~64: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t arrow. However if bloom-player is the top level window (i.e. it is not embedded in anot...(EN_COMPOUND_ADJECTIVE_INTERNAL)
public/testBooks/multibook-target1/origami.css (4)
46-48: Remove or update the commented-out code.The comment about JohnT's modification should either be removed or updated to follow a standard documentation format.
- /* JohnT mod: this extra padding makes it impossible to grab the crop control right above the split. - padding-bottom: 5px; /* this actually enlarges the target area for the user trying to find the splitter, without changing the layout of the page (because the position is absolute)*/ + /* Removed padding-bottom to prevent interference with crop control above the split */
205-209: Remove commented-out transition code.Either implement the transition or remove the commented code block.
-/* I wasn't convinced this was helpful... may return to it -.adders{ - transition-property: width, height; - transition-duration: 0.5s; -}*/
271-277: Improve documentation of z-index behavior.The comment about z-index behavior should be more clearly documented, possibly with a specific example.
.origami-layout-mode { - /*the commented line below would also add it to an empty bloom-content1 for re-entry to layout mode*/ - /*but that would be problematic/confusing in a multilingual context*/ - /*.origami-layout-mode .split-pane-component-inner .bloom-editable.bloom-content1:empty:before,*/ - /* not setting z-index in the upper layer allows the adder buttons to be visible and active when the */ - /* cursor is over the "Text Box" text, but not when it is over the .formatButton gearbox icon. (BL-9176) */ + /* Z-index hierarchy: + * - Adder buttons: visible when cursor is over "Text Box" + * - Format button (gearbox): takes precedence over adder buttons + * Note: Intentionally not applying to empty bloom-content1 to avoid multilingual confusion + * Reference: BL-9176 + */ }
304-304: Remove source map comment if not used.The source map comment should be removed if source maps are not being generated.
-/*# sourceMappingURL=../../../../output/browser/bookEdit/css/origami.css.map */public/testBooks/multibook-index/Basic Book.css (3)
1-348: Consider consolidating duplicate CSS filesThe same CSS file exists in both
multibook-indexandmultibook-target1directories. This duplication could lead to maintenance issues.Consider:
- Moving the shared CSS to a common location
- Using CSS imports or a build process to include the styles where needed
- Creating a shared component library for these styles
59-59: Simplify the calculation for top positioningThe calculation
calc(1.3% + 24% + 24%)could be simplified tocalc(49.3%)for better readability and performance.- top: calc(1.3% + 24% + 24%); + top: calc(49.3%);
14-14: Avoid using !important declarationsThe use of
!importantin border declarations suggests potential specificity issues in the CSS cascade.Consider:
- Restructuring the selectors to be more specific
- Using more specific combinators instead of !important
- Document why !important is necessary if it cannot be avoided
Also applies to: 138-138
src/externalContext.ts (1)
Line range hint
193-203: Consider enhancing error loggingThe JSON parse error handling could be more informative for debugging purposes.
} catch (e) { console.log( - "receiveMessage failed to parse json: " + - data + - " with error " + - e.message, + `receiveMessage failed to parse json: ${JSON.stringify(data)} with error: ${e.message}`, ); return; }src/bookInfo.ts (1)
24-24: Consider using a getter method instead of making bookInstanceId public.Making the property public breaks encapsulation. Consider using a getter method to maintain better control over the property access:
-public bookInstanceId = ""; +private bookInstanceId = ""; + +public getBookInstanceId(): string { + return this.bookInstanceId; +}public/testBooks/multibook-index/appearance.css (2)
115-116: Fix incomplete comment.The comment appears to be cut off mid-sentence: "In some themes, at edit-". Please complete the comment to improve code documentation.
177-177: Document the magic number for page number extra height.The value
8mmappears to be a magic number. Consider adding a comment explaining why this specific value was chosen for the extra height of page numbers on device pages.src/controlBar.tsx (3)
39-39: Remove unused import.The
Buttonimport from@material-ui/coreis not used in the code.-import { Button } from "@material-ui/core";
Line range hint
254-296: Simplify back button rendering logic.The nested conditional rendering with multiple checks against
BackButtonStatecould be simplified using a separate function or switch statement for better readability.Consider refactoring to:
const renderBackButton = (state: BackButtonState): JSX.Element | null => { switch (state) { case BackButtonState.showArrow: return ( <ArrowBack aria-label="Go Back" titleAccess={LocalizationManager.getTranslation( "Button.Back", props.preferredLanguages, "Back" )} /> ); case BackButtonState.showEllipsis: return ( <MoreHoriz aria-label="More Menu" titleAccess={LocalizationManager.getTranslation( "Button.More", props.preferredLanguages, "More" )} /> ); default: return null; } };Then use it in the JSX:
{!props.videoPreviewMode && ( <IconButton color="secondary" onClick={() => props.backClicked?.()} data-testid="history-back-button" disabled={props.getBackButtonState() === BackButtonState.showNothing} > {renderBackButton(props.getBackButtonState())} </IconButton> )}
258-262: Add type for onClick event handler.The onClick event handler should have proper type annotations for better type safety.
-onClick={() => { - if (props.backClicked) { - props.backClicked(); - } -}} +onClick={(e: React.MouseEvent<HTMLButtonElement>) => props.backClicked?.()}src/bloom-player-controls.tsx (2)
913-928: Consider adding documentation for the back button states.The
getBackButtonStatemethod implements a clear logic flow, but would benefit from JSDoc comments explaining:
- The purpose and use case for each state (showArrow, showEllipsis, showNothing)
- The conditions that trigger each state
- The difference between showing an arrow vs ellipsis
+/** + * Determines the visibility state of the back button based on navigation history and container context. + * @returns {BackButtonState} + * - showArrow: When there's internal history or the container wants a direct "back" action + * - showEllipsis: When in an iframe and the parent page handles navigation + * - showNothing: When no back navigation is available + */ getBackButtonState={() => {
934-938: Add error handling to the back button click handler.The back button click handler should include error handling to gracefully handle potential failures when sending messages to the host.
backClicked={() => { + try { if (!coreRef.current?.HandleBackButtonClicked()) { sendBackToHost(); } + } catch (error) { + console.error("Failed to handle back button click:", error); + // Fallback behavior or error notification + } }}public/testBooks/multibook-index/basePage.css (1)
1-1304: Consider performance optimizations for the CSS.The CSS file is comprehensive but could benefit from the following optimizations:
- Reduce specificity of selectors to improve rendering performance
- Consider splitting into smaller files for better caching
- Evaluate if all transform operations are necessary
- Group media queries to reduce CSS size
Consider implementing CSS modules or a CSS-in-JS solution for better code organization and performance. This would also help with:
- Dead code elimination
- Scoped styles
- Dynamic theming
- Better build-time optimizations
public/testBooks/multibook-target1/Basic Book.css (3)
3-7: Consider using relative units for better responsivenessThe fixed height of 1.6cm for
.translatorsmight not accommodate varying content lengths across different languages and screen sizes.Consider using relative units or min-height:
.translators { text-align: center; - height: 1.6cm; + min-height: 1.6cm; position: relative; }
8-153: Consider using CSS Grid or Flexbox for more maintainable layoutsThe current implementation heavily relies on absolute positioning, which can be brittle and harder to maintain. Modern CSS layout systems like Grid or Flexbox could provide more flexible and maintainable solutions.
Example approach using CSS Grid:
.imageOnTop { display: grid; grid-template-rows: auto 1fr; gap: var(--multilingual-editable-vertical-gap); } .imageOnTop .bloom-imageContainer { grid-row: 1; } .imageOnTop .bloom-translationGroup { grid-row: 2; }
154-163: Consider using CSS for vertical centeringThe comment indicates that vertical centering is handled in JavaScript. Modern CSS can handle this more efficiently.
Consider using CSS for vertical centering:
.textWholePage { + display: flex; + flex-direction: column; + justify-content: center; } .textWholePage .bloom-editable { min-height: 2em; } .textWholePage .bloom-translationGroup { width: 100%; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (16)
public/testBooks/multibook-index/BloomLocal.svgis excluded by!**/*.svgpublic/testBooks/multibook-index/back-cover-local.svgis excluded by!**/*.svgpublic/testBooks/multibook-index/happy-pirate.pngis excluded by!**/*.pngpublic/testBooks/multibook-index/history.dbis excluded by!**/*.dbpublic/testBooks/multibook-index/image.jpgis excluded by!**/*.jpgpublic/testBooks/multibook-index/license.pngis excluded by!**/*.pngpublic/testBooks/multibook-index/placeHolder.pngis excluded by!**/*.pngpublic/testBooks/multibook-index/thumbnail.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/BloomLocal.svgis excluded by!**/*.svgpublic/testBooks/multibook-target1/back-cover-local.svgis excluded by!**/*.svgpublic/testBooks/multibook-target1/history.dbis excluded by!**/*.dbpublic/testBooks/multibook-target1/image.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/license.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/placeHolder.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/thumbnail.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (45)
.storybook/main.ts(2 hunks).storybook/preview.ts(1 hunks)README-advanced.md(2 hunks)README.md(4 hunks)package.json(2 hunks)public/testBooks/multibook-index/Basic Book.css(1 hunks)public/testBooks/multibook-index/Traditional-XMatter.css(1 hunks)public/testBooks/multibook-index/appearance.css(1 hunks)public/testBooks/multibook-index/appearance.json(1 hunks)public/testBooks/multibook-index/basePage.css(1 hunks)public/testBooks/multibook-index/book.userPrefs(1 hunks)public/testBooks/multibook-index/branding.css(1 hunks)public/testBooks/multibook-index/customCollectionStyles.css(1 hunks)public/testBooks/multibook-index/defaultLangStyles.css(1 hunks)public/testBooks/multibook-index/index.htm(1 hunks)public/testBooks/multibook-index/meta.json(1 hunks)public/testBooks/multibook-index/origami.css(1 hunks)public/testBooks/multibook-index/previewMode.css(1 hunks)public/testBooks/multibook-index/publish-settings.json(1 hunks)public/testBooks/multibook-target1/Basic Book.css(1 hunks)public/testBooks/multibook-target1/Traditional-XMatter.css(1 hunks)public/testBooks/multibook-target1/appearance.css(1 hunks)public/testBooks/multibook-target1/appearance.json(1 hunks)public/testBooks/multibook-target1/basePage.css(1 hunks)public/testBooks/multibook-target1/book.userPrefs(1 hunks)public/testBooks/multibook-target1/branding.css(1 hunks)public/testBooks/multibook-target1/customCollectionStyles.css(1 hunks)public/testBooks/multibook-target1/defaultLangStyles.css(1 hunks)public/testBooks/multibook-target1/index.htm(1 hunks)public/testBooks/multibook-target1/meta.json(1 hunks)public/testBooks/multibook-target1/origami.css(1 hunks)public/testBooks/multibook-target1/previewMode.css(1 hunks)public/testBooks/multibook-target1/publish-settings.json(1 hunks)src/bloom-player-controls.tsx(4 hunks)src/bloom-player-core.tsx(25 hunks)src/bloomPlayerAnalytics.tsx(1 hunks)src/bookInfo.ts(1 hunks)src/controlBar.tsx(5 hunks)src/externalContext.ts(1 hunks)src/navigation.test.ts(1 hunks)src/navigation.ts(1 hunks)src/stories/BloomPlayerIframe.tsx(1 hunks)src/stories/BloomPlayerTester.ts(1 hunks)src/stories/multiBook.stories.tsx(1 hunks)src/stories/navigation.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (16)
- public/testBooks/multibook-target1/customCollectionStyles.css
- public/testBooks/multibook-index/customCollectionStyles.css
- public/testBooks/multibook-target1/appearance.json
- public/testBooks/multibook-index/appearance.json
- public/testBooks/multibook-index/defaultLangStyles.css
- public/testBooks/multibook-target1/defaultLangStyles.css
- public/testBooks/multibook-index/book.userPrefs
- public/testBooks/multibook-index/previewMode.css
- public/testBooks/multibook-target1/book.userPrefs
- public/testBooks/multibook-index/Traditional-XMatter.css
- src/bloomPlayerAnalytics.tsx
- public/testBooks/multibook-index/publish-settings.json
- public/testBooks/multibook-index/meta.json
- public/testBooks/multibook-index/origami.css
- public/testBooks/multibook-target1/publish-settings.json
- public/testBooks/multibook-target1/meta.json
🧰 Additional context used
🪛 LanguageTool
README-advanced.md
[style] ~28-~28: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...is designed to be published using npm so as to be readily available to a variety of cl...
(SO_AS_TO)
[style] ~28-~28: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ... npm so as to be readily available to a variety of clients. The published version includes...
(A_VARIETY_OF)
[grammar] ~28-~28: Probable usage error. Use “and” after ‘both’.
Context: ...cludes both dist/bloomPlayer-HASH.js, as well as some mp3 files used by activities.
W...
(BOTH_AS_WELL_AS)
README.md
[uncategorized] ~57-~57: Consider adding a hyphen.
Context: ...tton If true, displays a button in the upper left corner of the app bar. When clicked, bl...
(TOP_LEFT_CORNER)
[grammar] ~57-~57: A determiner cannot be combined with a possessive pronoun. Did you mean simply “the” or “your”?
Context: ...post a message "backButtonClicked" that the your host can catch and respond to. Example...
(A_MY)
[uncategorized] ~64-~64: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...mally this button will be a left arrow. However if bloom-player is the top level window...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~64-~64: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t arrow. However if bloom-player is the top level window (i.e. it is not embedded in anot...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[formatting] ~106-~106: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...r. However, Bloom Player needs your help, because it does not have a way to know what the...
(COMMA_BEFORE_BECAUSE)
[uncategorized] ~158-~158: Possible missing comma found.
Context: ...y. ### Running unit tests To run unit tests use yarn test. This will run all `*.t...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint (0.35.0)
README.md
109-109: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
🪛 Biome (1.9.4)
src/bloom-player-core.tsx
[error] 1898-1898: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (19)
public/testBooks/multibook-target1/Traditional-XMatter.css (1)
227-243: LGTM! Consistent implementation.
The back cover layout follows the same patterns as the front cover, maintaining consistency throughout the codebase.
public/testBooks/multibook-target1/appearance.css (3)
122-124: LGTM! Clear and purposeful override for interactive pages.
The transparent marginBox for interactive pages is well-implemented and properly documented.
195-204: LGTM! Well-implemented specificity handling.
The doubled class names are a clever solution for specificity management and are well-documented.
209-217: LGTM! Clear and well-documented XMatter styles.
The variables are logically named and the font size inheritance is well explained.
public/testBooks/multibook-target1/previewMode.css (2)
115-115: Review source map path resolution
The source map uses a relative path with multiple parent directory references (../../../../). This could be fragile across different environments. Consider using a more robust path resolution strategy.
60-65: Document and review wkhtmltopdf version dependency
The CSS contains a hack specific to wkhtmltopdf 0.10.0. This version-specific dependency should be documented and periodically reviewed as it might not be necessary with newer versions of the tool.
✅ Verification successful
Version-specific CSS hack is documented and appears to be a common requirement
The CSS hack for wkhtmltopdf 0.10.0 is actually well documented across multiple files in the codebase. The comments explain that this invisible border is necessary for proper page layout with wkhtmltopdf 0.10.0, as without it "pages would not come out properly at all." This appears to be an intentional and understood workaround rather than an undocumented dependency.
- Found consistent documentation in multiple basePage.css files explaining the necessity of the border for proper PDF generation
- The hack appears to be a core layout requirement across different book templates
- The version dependency is explicitly documented in the comments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any comments or documentation about wkhtmltopdf version requirements
rg -i "wkhtmltopdf" --type md
Length of output: 29
Script:
#!/bin/bash
# Let's search for wkhtmltopdf mentions across all file types to understand its usage
rg -i "wkhtmltopdf" -A 2 -B 2
# Also search for any build or dependency configuration files that might specify the version
fd -e json -e config -e yml -e yaml | xargs rg -i "wkhtmltopdf"
Length of output: 8741
public/testBooks/multibook-target1/index.htm (1)
417-421: Review navigation implementation for maintainability.
The current navigation implementation has several concerns:
- Hardcoded book IDs in URLs could break if books are reorganized
- Multiple ways to navigate to the same page could lead to inconsistent behavior
- Back navigation mentioned in content implies UI elements that should be verified
public/testBooks/multibook-target1/branding.css (1)
14-21: Verify accessibility requirements for branding images
Ensure that branding images have appropriate alt text requirements defined in the HTML.
✅ Verification successful
Empty alt attributes are appropriate for decorative branding images
The branding images in the codebase are consistently implemented with empty alt attributes (alt=""), which is the correct accessibility practice for decorative images that don't convey meaningful content. These images appear to be logos and branding elements that are purely presentational, and empty alt text ensures screen readers will correctly skip them.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if branding images have alt attributes
rg -l 'img.*class="branding"' | xargs rg -l 'alt="'
Length of output: 902
Script:
#!/bin/bash
# Let's check for branding-related images and their alt attributes in the HTML files
rg -U --multiline 'data-book.*branding.*?<img.*?>' -g '*.htm' -g '*.html'
Length of output: 2166
.storybook/main.ts (3)
10-12: Approved: Added static directory configuration for Storybook
The addition of staticDirs: ["../dist"], properly configures Storybook to serve static files from the dist directory. This is necessary for stories that include an iframe and need access to bloomplayer.htm and its bundles.
28-30: Verify the proxy configuration for /book/ route
The comment indicates that the proxy setup for /book/ is not working. Please verify the proxy configuration to ensure it correctly handles requests and resolves any CORS issues.
53-59: Verify the proxy target file:/// for /bloom/ route
Setting the proxy target to file:/// may not function as intended, as proxying to the file protocol is uncommon and might not be supported. Please verify if this configuration works as expected or consider alternative methods to serve static files.
.storybook/preview.ts (1)
12-16: Approved: Customized story sorting in Storybook
The addition of options.storySort.order configures Storybook to sort stories based on the specified order, improving the organization of the stories.
package.json (2)
26-27: LGTM: New script for Storybook iframe development
The new buildForStorybook script is well-documented and serves a clear purpose for stories that use an iframe and bloomplayer.htm instead of raw React components.
47-53: Verify Storybook dependency compatibility
The Storybook dependencies have been updated from 8.3.6 to 8.4.6. While this is a minor version bump, it's good practice to verify compatibility.
Also applies to: 61-61, 67-67
✅ Verification successful
Storybook 8.4.6 update is safe to proceed
Based on the release notes, version 8.4.6 contains only improvements and compatibility updates:
- Better Windows support for Addon Test
- Angular v19 standalone components support
- Vite 6 support
- Enhanced portable stories functionality
- React-docgen-typescript upgrade for Vite 6
- Svelte plugin v5 support
No breaking changes are mentioned, and the changes are focused on adding support for newer versions of related tools.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in Storybook 8.4.6 release notes
gh api graphql -f query='
{
repository(owner: "storybookjs", name: "storybook") {
release(tagName: "v8.4.6") {
description
}
}
}'
Length of output: 1040
src/navigation.test.ts (2)
8-14: LGTM: Good test setup with proper cleanup.
The beforeEach hook ensures a clean state for each test by clearing the navigation history.
107-134: Well-structured multi-step navigation test.
Comprehensive test covering multiple navigation scenarios and verifying the correct state after each step.
README.md (1)
102-130: Well-documented multi-book support section.
Clear explanation of the Instance ID system and the required host implementation steps.
🧰 Tools
🪛 LanguageTool
[formatting] ~106-~106: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...r. However, Bloom Player needs your help, because it does not have a way to know what the...
(COMMA_BEFORE_BECAUSE)
🪛 Markdownlint (0.35.0)
109-109: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
src/externalContext.ts (1)
116-116: Verify all usages of the renamed function
The function has been renamed from onBackClicked to sendBackToHost for better clarity. Ensure all call sites have been updated.
✅ Verification successful
All usages of the renamed function have been properly updated
The function rename from onBackClicked to sendBackToHost has been correctly implemented across the codebase:
- The only remaining reference to
onBackClickedis in a comment insrc/externalContext.ts - The new function name
sendBackToHostis properly used in:- Export declaration in
src/externalContext.ts - Import and two usage locations in
src/bloom-player-controls.tsx
- Export declaration in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to onBackClicked
# Test: Search for old function name references
rg "onBackClicked"
# Test: Verify the new function name usage
rg "sendBackToHost"
Length of output: 317
public/testBooks/multibook-target1/Basic Book.css (1)
1-348: Document CSS custom properties and verify source map
Two concerns need attention:
- The CSS variable
--multilingual-editable-vertical-gapis used but not defined in this file - Source map is referenced but not provided
Let's check for the CSS variable definition and source map:
| order: kFrontCoverImageOrder; | ||
| flex: 1 1 auto; |
There was a problem hiding this comment.
Fix invalid CSS value kFrontCoverImageOrder
The value kFrontCoverImageOrder appears to be intended as a CSS variable but is used directly. This will cause the order property to be invalid.
Apply this fix:
- order: kFrontCoverImageOrder;
+ order: var(--kFrontCoverImageOrder);Also applies to: 58-59
There was a problem hiding this comment.
we don't want to review html or css in this pr
There was a problem hiding this comment.
@hatton, understood. I will skip reviewing HTML and CSS files in this PR.
| .ISBNContainer .bloom-editable { | ||
| text-align: left !important; | ||
| width: 4.3cm !important; | ||
| max-height: 1.8em; | ||
| min-height: 1.8em; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making ISBN dimensions more flexible
The hard-coded dimensions and use of !important might cause maintainability issues:
- Fixed width of 4.3cm might not accommodate all ISBN formats
- Fixed height of 1.8em might clip text with different font sizes
- Use of
!importantsuggests potential specificity issues
Consider using:
.ISBNContainer .bloom-editable {
text-align: left;
min-width: 4.3cm;
max-width: fit-content;
min-height: 1.8em;
height: auto;
}There was a problem hiding this comment.
we don't want to review html or css in this pr
There was a problem hiding this comment.
Rate Limit Exceeded
@hatton have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 22 seconds before sending another message.
| .outsideBackCover img.branding { | ||
| margin-top: 4px; | ||
| width: 10cm; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the use of absolute units
Using centimeters (cm) for digital displays can be unpredictable across different devices and screen resolutions.
Consider using relative units:
.outsideBackCover img.branding {
margin-top: 4px;
- width: 10cm;
+ width: min(10cm, 30%); /* Provides a relative fallback */
+ max-width: 10cm;
}Also, consider using a more specific class name instead of combining element and class selectors for better maintainability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .outsideBackCover img.branding { | |
| margin-top: 4px; | |
| width: 10cm; | |
| } | |
| .outsideBackCover img.branding { | |
| margin-top: 4px; | |
| width: min(10cm, 30%); /* Provides a relative fallback */ | |
| max-width: 10cm; | |
| } |
There was a problem hiding this comment.
@hatton, understood. Let me know if you need any further assistance.
There was a problem hiding this comment.
Rate Limit Exceeded
@hatton have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 53 seconds before sending another message.
| /* these rules allow different branding insertions depending on orientation */ | ||
| .portraitOnly, | ||
| .landscapeOnly { | ||
| display: none; | ||
| } | ||
| .Size6x9Portrait .portraitOnly, | ||
| .USComicPortrait .portraitOnly, | ||
| .Device16x9Portrait .portraitOnly, | ||
| .QuarterLetterPortrait .portraitOnly, | ||
| .HalfLetterPortrait .portraitOnly, | ||
| .LetterPortrait .portraitOnly, | ||
| .LegalPortrait .portraitOnly, | ||
| .A3Portrait .portraitOnly, | ||
| .A4Portrait .portraitOnly, | ||
| .A5Portrait .portraitOnly, | ||
| .B5Portrait .portraitOnly, | ||
| .A6Portrait .portraitOnly { | ||
| display: unset; | ||
| } | ||
| .Size6x9Landscape .landscapeOnly, | ||
| .Cm13Landscape .landscapeOnly, | ||
| .Device16x9Landscape .landscapeOnly, | ||
| .LetterLandscape .landscapeOnly, | ||
| .LegalLandscape .landscapeOnly, | ||
| .A3Landscape .landscapeOnly, | ||
| .A4Landscape .landscapeOnly, | ||
| .A5Landscape .landscapeOnly, | ||
| .A6Landscape .landscapeOnly { | ||
| display: unset; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize orientation-specific display rules
The current implementation could be improved in several ways:
display: unsetis less predictable than explicit values- The extensive list of selectors could be simplified using CSS custom properties
Consider this alternative approach:
- .portraitOnly,
- .landscapeOnly {
- display: none;
- }
+ :root {
+ --portrait-display: none;
+ --landscape-display: none;
+ }
+ /* Base classes */
+ .portraitOnly {
+ display: var(--portrait-display, none);
+ }
+ .landscapeOnly {
+ display: var(--landscape-display, none);
+ }
+ /* Portrait sizes */
+ [class*="Portrait"] {
+ --portrait-display: block;
+ }
+ /* Landscape sizes */
+ [class*="Landscape"] {
+ --landscape-display: block;
+ }This approach:
- Uses explicit display values
- Reduces code duplication
- Is easier to maintain
- Provides better browser compatibility than
unset
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* these rules allow different branding insertions depending on orientation */ | |
| .portraitOnly, | |
| .landscapeOnly { | |
| display: none; | |
| } | |
| .Size6x9Portrait .portraitOnly, | |
| .USComicPortrait .portraitOnly, | |
| .Device16x9Portrait .portraitOnly, | |
| .QuarterLetterPortrait .portraitOnly, | |
| .HalfLetterPortrait .portraitOnly, | |
| .LetterPortrait .portraitOnly, | |
| .LegalPortrait .portraitOnly, | |
| .A3Portrait .portraitOnly, | |
| .A4Portrait .portraitOnly, | |
| .A5Portrait .portraitOnly, | |
| .B5Portrait .portraitOnly, | |
| .A6Portrait .portraitOnly { | |
| display: unset; | |
| } | |
| .Size6x9Landscape .landscapeOnly, | |
| .Cm13Landscape .landscapeOnly, | |
| .Device16x9Landscape .landscapeOnly, | |
| .LetterLandscape .landscapeOnly, | |
| .LegalLandscape .landscapeOnly, | |
| .A3Landscape .landscapeOnly, | |
| .A4Landscape .landscapeOnly, | |
| .A5Landscape .landscapeOnly, | |
| .A6Landscape .landscapeOnly { | |
| display: unset; | |
| } | |
| /* these rules allow different branding insertions depending on orientation */ | |
| :root { | |
| --portrait-display: none; | |
| --landscape-display: none; | |
| } | |
| /* Base classes */ | |
| .portraitOnly { | |
| display: var(--portrait-display, none); | |
| } | |
| .landscapeOnly { | |
| display: var(--landscape-display, none); | |
| } | |
| /* Portrait sizes */ | |
| [class*="Portrait"] { | |
| --portrait-display: block; | |
| } | |
| /* Landscape sizes */ | |
| [class*="Landscape"] { | |
| --landscape-display: block; | |
| } |
There was a problem hiding this comment.
@hatton, understood. I'll note your preference to keep the current implementation.
✏️ Learnings added
Learnt from: hatton
PR: BloomBooks/bloom-player#347
File: public/testBooks/multibook-index/branding.css:25-54
Timestamp: 2024-12-10T16:30:37.960Z
Learning: In `public/testBooks/multibook-index/branding.css`, when handling orientation-specific display rules for the `.portraitOnly` and `.landscapeOnly` classes, the preferred approach is to use extensive selectors with `display: unset` instead of refactoring to use CSS custom properties.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Rate Limit Exceeded
@hatton have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 53 seconds before sending another message.
| .A3Landscape, | ||
| .A3Portrait, | ||
| .A4Landscape, | ||
| .A4Portrait, | ||
| .B5Portrait, | ||
| .LetterLandscape, | ||
| .LetterPortrait, | ||
| .LegalLandscape, | ||
| .LegalLandscape { | ||
| --page-margin: 15mm; | ||
| --page-gutter: 10mm; | ||
| } |
There was a problem hiding this comment.
Remove duplicate LegalLandscape class.
The class list contains a duplicate entry for LegalLandscape.
.A4Portrait,
.B5Portrait,
.LetterLandscape,
.LetterPortrait,
.LegalLandscape,
-.LegalLandscape {
+.LegalPortrait {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .A3Landscape, | |
| .A3Portrait, | |
| .A4Landscape, | |
| .A4Portrait, | |
| .B5Portrait, | |
| .LetterLandscape, | |
| .LetterPortrait, | |
| .LegalLandscape, | |
| .LegalLandscape { | |
| --page-margin: 15mm; | |
| --page-gutter: 10mm; | |
| } | |
| .A3Landscape, | |
| .A3Portrait, | |
| .A4Landscape, | |
| .A4Portrait, | |
| .B5Portrait, | |
| .LetterLandscape, | |
| .LetterPortrait, | |
| .LegalLandscape, | |
| .LegalPortrait { | |
| --page-margin: 15mm; | |
| --page-gutter: 10mm; | |
| } |
There was a problem hiding this comment.
@hatton, understood. Skipping this issue.
| play: async ({ canvasElement, step }) => { | ||
| const tester = new BloomPlayerTester(canvasElement); | ||
|
|
||
| await step("Jump to a certain page in another book", async () => { | ||
| await tester.clickLinkByText("Crab Page"); | ||
| await tester.shouldSeeText("This is the crab page the green book"); | ||
| }); | ||
|
|
||
| await step( | ||
| "Swipe pages to the back cover and then follow a link to jump to front cover", | ||
| async () => { | ||
| await tester.goToNextPage(); | ||
| await tester.goToNextPage(); | ||
| await tester.shouldSeeText( | ||
| "This is the back cover of the green book", | ||
| ); | ||
| await tester.clickLinkByText("Jump to front cover"); | ||
| }, | ||
| ); | ||
| await step( | ||
| "Jump back to the back cover and click a link within this book to the crab page", | ||
| async () => { | ||
| await tester.clickBackButton(); | ||
| await tester.shouldSeeText( | ||
| "This is the back cover of the green book", | ||
| ); | ||
| }, | ||
| ); | ||
| await step( | ||
| "Use a link that has the book and page id to jump to the crab page", | ||
| async () => { | ||
| await tester.clickLinkByText("using /book/bookId#pageId"); | ||
| await tester.shouldSeeText( | ||
| "This is the crab page the green book", | ||
| ); | ||
| }, | ||
| ); | ||
| await step( | ||
| 'Use a link with a special "#back" href to get back to the back cover', | ||
| async () => { | ||
| await tester.shouldSeeText( | ||
| "This is the crab page the green book", | ||
| ); | ||
| await tester.clickLinkByText('this "back" link'); | ||
| await tester.shouldSeeText( | ||
| "This is the back cover of the green book", | ||
| ); | ||
| }, | ||
| ); | ||
| await step("Jump back to get back to the original book", async () => { | ||
| await tester.clickBackButton(); | ||
| await tester.shouldSeeText( | ||
| "This is a page in the index book that has links to another book", | ||
| ); | ||
| }); | ||
|
|
||
| await step("Jump to this book's cover", async () => { | ||
| await tester.clickLinkByText("This book Cover"); | ||
| await tester.shouldSeeText("multibook-index"); | ||
| }); | ||
| await step( | ||
| "Jump back and verify that the back button is gone", | ||
| async () => { | ||
| await tester.clickBackButton(); | ||
| await tester.waitForHistoryBackButtonToGoAway(); | ||
| }, | ||
| ); | ||
| await step("Show the success page", async () => { | ||
| await tester.goToNextPage(); | ||
| await tester.shouldSeeText("This is the end."); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error scenarios to navigation tests
The current test scenarios cover the happy path well, but should also include error cases:
- Invalid URLs
- Missing pages
- Network errors
- History stack overflow
export const Default: Story = {
args: {
bookUrl:
"testBooks/multibook-index/index.htm#653f29c1-665b-4d63-a061-086401abc106",
showBackButton: false,
},
play: async ({ canvasElement, step }) => {
const tester = new BloomPlayerTester(canvasElement);
+
+ await step("Handle invalid book URL", async () => {
+ await tester.clickLinkByText("Invalid Book");
+ await tester.shouldSeeErrorMessage("Book not found");
+ });
+
+ await step("Handle missing page", async () => {
+ await tester.clickLinkByText("Missing Page");
+ await tester.shouldSeeErrorMessage("Page not found");
+ });Committable suggestion skipped: line range outside the PR's diff.
| .A3Landscape, | ||
| .A3Portrait, | ||
| .A4Landscape, | ||
| .A4Portrait, | ||
| .B5Portrait, | ||
| .LetterLandscape, | ||
| .LetterPortrait, | ||
| .LegalLandscape, | ||
| .LegalLandscape { | ||
| --page-margin: 15mm; | ||
| --page-gutter: 10mm; | ||
| } |
There was a problem hiding this comment.
Remove duplicate .LegalLandscape selector.
The .LegalLandscape selector appears twice in the list, which could lead to confusion.
.A4Portrait,
.B5Portrait,
.LetterLandscape,
.LetterPortrait,
-.LegalLandscape,
.LegalLandscape {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .A3Landscape, | |
| .A3Portrait, | |
| .A4Landscape, | |
| .A4Portrait, | |
| .B5Portrait, | |
| .LetterLandscape, | |
| .LetterPortrait, | |
| .LegalLandscape, | |
| .LegalLandscape { | |
| --page-margin: 15mm; | |
| --page-gutter: 10mm; | |
| } | |
| .A3Landscape, | |
| .A3Portrait, | |
| .A4Landscape, | |
| .A4Portrait, | |
| .B5Portrait, | |
| .LetterLandscape, | |
| .LetterPortrait, | |
| .LegalLandscape { | |
| --page-margin: 15mm; | |
| --page-gutter: 10mm; | |
| } |
There was a problem hiding this comment.
@hatton, understood.
✏️ Learnings added
Learnt from: hatton
PR: BloomBooks/bloom-player#347
File: public/testBooks/multibook-index/appearance.css:129-140
Timestamp: 2024-12-10T16:27:54.160Z
Learning: In the CSS files (e.g., `appearance.css`) in this project, duplicate CSS selectors (such as multiple `.LegalLandscape` selectors) may be intentional and should not be flagged as issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Rate Limit Exceeded
@hatton have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 52 seconds before sending another message.
| @@ -1014,7 +1038,7 @@ export const BloomPlayerControls: React.FunctionComponent<BloomPlayerProps> = ( | |||
| } | |||
| }} | |||
| shouldReportSoundLog={props.shouldReportSoundLog} | |||
| startPage={props.startPage} | |||
| startPageIndex={props.startPage} | |||
There was a problem hiding this comment.
💡 Codebase verification
Breaking change detected: startPage prop is still in use
The codebase shows inconsistent prop naming, which needs to be addressed:
BloomPlayerCoreusesstartPageIndexBloomPlayerControlsstill usesstartPageinternally and passes it asstartPageIndexto core- Multiple files contain references to the old
startPageprop name
This indicates a breaking change that's not fully implemented across the codebase. The changes needed:
- Update
BloomPlayerControlsinterface to usestartPageIndex - Update all internal references in
BloomPlayerControlsfromstartPagetostartPageIndex - Update any consuming components still using
startPage
🔗 Analysis chain
Verify the impact of renaming startPage to startPageIndex.
The property rename from startPage to startPageIndex improves clarity but may be a breaking change for existing consumers.
Run the following script to check for existing usages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usages of the old startPage prop
# Test: Search for startPage prop usage. Expect: Only occurrences of the new name.
echo "Checking for old prop name usage..."
rg "startPage[^I]" --type ts --type tsx
echo "Checking for new prop name usage..."
rg "startPageIndex" --type ts --type tsx
Length of output: 300
Script:
#!/bin/bash
# Let's try without file type filters since tsx is not recognized
echo "Checking for old prop name usage..."
rg "startPage[^I]"
echo "Checking for new prop name usage..."
rg "startPageIndex"
Length of output: 3139
| .pictureAndWordPage.QuarterLetterLandscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.QuarterLetterLandscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.HalfLetterPortrait .bloom-translationGroup { | ||
| height: 35%; | ||
| } | ||
| .pictureAndWordPage.HalfLetterPortrait .bloom-imageContainer { | ||
| height: 62%; | ||
| } | ||
| .pictureAndWordPage.HalfLetterPortrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.LetterPortrait .bloom-translationGroup { | ||
| height: 23%; | ||
| } | ||
| .pictureAndWordPage.LetterPortrait .bloom-imageContainer { | ||
| height: 74%; | ||
| } | ||
| .pictureAndWordPage.LetterPortrait .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.LetterLandscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.LetterLandscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.LetterLandscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.LegalPortrait .bloom-translationGroup { | ||
| height: 23%; | ||
| } | ||
| .pictureAndWordPage.LegalPortrait .bloom-imageContainer { | ||
| height: 74%; | ||
| } | ||
| .pictureAndWordPage.LegalPortrait .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.LegalLandscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.LegalLandscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.LegalLandscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.Cm13Landscape .bloom-translationGroup { | ||
| height: 37%; | ||
| } | ||
| .pictureAndWordPage.Cm13Landscape .bloom-imageContainer { | ||
| height: 60%; | ||
| } | ||
| .pictureAndWordPage.Cm13Landscape .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.USComicPortrait .bloom-translationGroup { | ||
| height: 32%; | ||
| } | ||
| .pictureAndWordPage.USComicPortrait .bloom-imageContainer { | ||
| height: 65%; | ||
| } | ||
| .pictureAndWordPage.USComicPortrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Portrait .bloom-translationGroup { | ||
| height: 35%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Portrait .bloom-imageContainer { | ||
| height: 62%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Portrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Landscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Landscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Landscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage .bloom-translationGroup { | ||
| width: 100%; | ||
| text-align: center; | ||
| padding-top: 10px; | ||
| } | ||
| .pictureAndWordPage .bloom-editable { | ||
| vertical-align: middle; | ||
| height: auto; | ||
| } | ||
| .pictureAndWordPage .bloom-content2, | ||
| .pictureAndWordPage .bloom-content3 { | ||
| font-size: 200%; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication using CSS custom properties
The styles for different page formats contain significant repetition. This could be simplified using CSS custom properties and reducing magic numbers.
Consider refactoring to use CSS custom properties:
:root {
--translation-group-height-portrait: 35%;
--translation-group-height-landscape: 42%;
--image-container-height-portrait: 62%;
--image-container-height-landscape: 55%;
--font-size-large: 600%;
--font-size-medium: 400%;
}
.pictureAndWordPage[class*="Portrait"] .bloom-translationGroup {
height: var(--translation-group-height-portrait);
}
.pictureAndWordPage[class*="Portrait"] .bloom-imageContainer {
height: var(--image-container-height-portrait);
}
.pictureAndWordPage[class*="Landscape"] .bloom-translationGroup {
height: var(--translation-group-height-landscape);
}
.pictureAndWordPage[class*="Landscape"] .bloom-imageContainer {
height: var(--image-container-height-landscape);
}This approach would:
- Make the code more maintainable
- Make it easier to adjust values globally
- Document the purpose of specific values
- Reduce the likelihood of inconsistencies
There was a problem hiding this comment.
we don't want to review html or css in this pr
There was a problem hiding this comment.
Rate Limit Exceeded
@hatton have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 0 seconds before sending another message.
| .nospecidalmovement_imageOnTop .bloom-imageContainer { | ||
| height: calc(45% - var(--multilingual-editable-vertical-gap)); | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| top: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-translationGroup { | ||
| height: 50%; | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| bottom: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-editable.bloom-monolingual { | ||
| height: 100%; | ||
| } | ||
| .bloom-bilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(50% - var(--multilingual-editable-vertical-gap)); | ||
| } | ||
| .bloom-trilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(33% - var(--multilingual-editable-vertical-gap)); | ||
| } |
There was a problem hiding this comment.
Fix typo in class name 'nospecidalmovement'
There appears to be a typo in the class name 'nospecidalmovement' (should be 'nospecialmovement').
Apply this correction throughout the file:
-.nospecidalmovement_imageOnTop
+.nospecialmovement_imageOnTop📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .nospecidalmovement_imageOnTop .bloom-imageContainer { | |
| height: calc(45% - var(--multilingual-editable-vertical-gap)); | |
| width: 100%; | |
| position: absolute; | |
| left: 0; | |
| top: 0; | |
| } | |
| .nospecidalmovement_imageOnTop .bloom-translationGroup { | |
| height: 50%; | |
| width: 100%; | |
| position: absolute; | |
| left: 0; | |
| bottom: 0; | |
| } | |
| .nospecidalmovement_imageOnTop .bloom-editable.bloom-monolingual { | |
| height: 100%; | |
| } | |
| .bloom-bilingual.nospecidalmovement_imageOnTop .bloom-editable { | |
| height: calc(50% - var(--multilingual-editable-vertical-gap)); | |
| } | |
| .bloom-trilingual.nospecidalmovement_imageOnTop .bloom-editable { | |
| height: calc(33% - var(--multilingual-editable-vertical-gap)); | |
| } | |
| .nospecialmovement_imageOnTop .bloom-imageContainer { | |
| height: calc(45% - var(--multilingual-editable-vertical-gap)); | |
| width: 100%; | |
| position: absolute; | |
| left: 0; | |
| top: 0; | |
| } | |
| .nospecialmovement_imageOnTop .bloom-translationGroup { | |
| height: 50%; | |
| width: 100%; | |
| position: absolute; | |
| left: 0; | |
| bottom: 0; | |
| } | |
| .nospecialmovement_imageOnTop .bloom-editable.bloom-monolingual { | |
| height: 100%; | |
| } | |
| .bloom-bilingual.nospecialmovement_imageOnTop .bloom-editable { | |
| height: calc(50% - var(--multilingual-editable-vertical-gap)); | |
| } | |
| .bloom-trilingual.nospecialmovement_imageOnTop .bloom-editable { | |
| height: calc(33% - var(--multilingual-editable-vertical-gap)); | |
| } |
There was a problem hiding this comment.
@hatton, understood. I've noted your preference to keep the class name as nospecidalmovement_imageOnTop.
✏️ Learnings added
Learnt from: hatton
PR: BloomBooks/bloom-player#347
File: public/testBooks/multibook-target1/Basic Book.css:65-87
Timestamp: 2024-12-10T16:44:02.902Z
Learning: In `public/testBooks/multibook-target1/Basic Book.css`, the CSS class `nospecidalmovement_imageOnTop` is intentionally named and should not be corrected to `nospecialmovement_imageOnTop`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
hatton
force-pushed
the
pageAndBookNavigation
branch
from
d74b943 to
2226513
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (23)
public/testBooks/multibook-target1/Traditional-XMatter.css (2)
94-96: Consider removing!importantflagThe use of
!importantin.coverBottomLangNameline-height suggests potential specificity issues. Consider restructuring the CSS selectors to achieve the desired specificity without using!important.-.frontCover .bottomBlock .bottomTextContent .bottomRow .coverBottomLangName { - line-height: 1.6em !important; -} +/* Increase selector specificity instead */ +.frontCover .bottomBlock .bottomTextContent .bottomRow .coverBottomLangName.bloom-editable { + line-height: 1.6em; +}
1-262: Consider adding CSS documentationThis is a complex stylesheet that manages various book layout components. Consider adding documentation comments that:
- Explain the purpose and structure of each major section
- Document the expected HTML structure
- List any required CSS variables
- Provide usage examples
This will help future maintainers understand the styling system more easily.
public/testBooks/multibook-target1/previewMode.css (5)
16-24: Consider using device-specific classes instead of hardcoded IDsThe selector
.preview:not([data-l1]) .bloom-frontMatter.Device16x9Landscape#ddd9b811-63d6-4d51-baf8-0a5fe88cd73duses a hardcoded ID which could make the styles less maintainable and reusable.Consider refactoring to use device-specific classes:
-.preview:not([data-l1]) .bloom-frontMatter.Device16x9Landscape#ddd9b811-63d6-4d51-baf8-0a5fe88cd73d { +.preview:not([data-l1]) .bloom-frontMatter.Device16x9Landscape {
36-46: Consider using CSS custom properties for consistent opacity valuesUsing CSS variables would make it easier to maintain consistent opacity values across different disabled states.
Consider refactoring to:
+:root { + --disabled-opacity: 0.4; +} .disabledVisual img { - opacity: 0.4; + opacity: var(--disabled-opacity); } .disabledVisual div { - opacity: 0.4; + opacity: var(--disabled-opacity); cursor: not-allowed; }
51-54: Improve documentation of JavaScript dependencyThe comment indicates a JavaScript dependency for readonly state, but it could be more explicit.
Consider enhancing the comment to:
- /*we have javascript function which notices this border and makes it read-only - (which isn't a style, so we can't set it ourselves*/ + /* IMPORTANT: The transparent border is detected by JavaScript to set readonly state. + * This cannot be handled via CSS alone due to readonly being a property, not a style. + * See: [add link to relevant JavaScript file/function] */
55-65: Document browser/tool compatibility requirementsThe comments mention specific wkhtml version requirements, but this could be more comprehensive.
Consider adding a detailed comment block:
/* Browser/Tool Compatibility Requirements * - wkhtml v0.10.0: Requires invisible border for correct page layout * - Add other known browser/tool requirements here */
106-113: Consider implementing z-index management systemUsing a high z-index value (10000) could lead to stacking context issues. Consider implementing a z-index management system using CSS variables.
Example implementation:
+:root { + --z-index-overlay: 100; + --z-index-tooltip: 200; + --z-index-modal: 300; +} .preview.template .numberedPage .pageLabel, .bloomPlayer-page.template .numberedPage .pageLabel { position: absolute; - z-index: 10000; + z-index: var(--z-index-overlay); /* ... other properties ... */ }public/testBooks/multibook-target1/basePage.css (3)
20-37: Consider using CSS custom properties for image container stylesThe image container styles could be more maintainable by using CSS custom properties for repeated values.
+:root { + --image-fit: contain; + --image-dimensions: 100%; +} .bloom-imageContainer img { - width: 100%; - height: 100%; - object-fit: contain; + width: var(--image-dimensions); + height: var(--image-dimensions); + object-fit: var(--image-fit); }
261-278: Improve maintainability of transform calculationsConsider using CSS custom properties to make transform calculations more maintainable and reusable.
+:root { + --transform-scale-a5: 1.04054054; + --transform-scale-a4: 1.02857143; + --transform-scale-a3: 1.02020202; + --transform-offset: -1.25675676mm; +} .bloom-fullBleed .bloom-page.A5Portrait { - transform: translate(0, -1.25675676mm) scale(1.04054054); + transform: translate(0, var(--transform-offset)) scale(var(--transform-scale-a5)); } .bloom-fullBleed .bloom-page.A5Landscape { - transform: translate(-1.25675676mm, 0) scale(1.04054054); + transform: translate(var(--transform-offset), 0) scale(var(--transform-scale-a5)); }
1219-1259: Optimize duplicate margin calculationsConsider extracting common margin calculations into reusable CSS custom properties to reduce code duplication.
+.marginBox { + --margin-calculation: calc(var(--page-margin-left) * var(--page-and-marginBox-are-same-color-multiplicand)); + --page-number-margin: calc(var(--pageNumber-extra-height) * var(--pageNumber-show-multiplicand)); +} .marginBox > .split-pane-component-inner > .bloom-translationGroup { - padding-left: calc(var(--topLevel-text-padding-left) - (var(--page-margin-left) * var(--page-and-marginBox-are-same-color-multiplicand))); + padding-left: calc(var(--topLevel-text-padding-left) - var(--margin-calculation)); /* Apply similar changes to other padding calculations */ }public/testBooks/multibook-index/Basic Book.css (1)
164-347: Reduce code duplication using CSS custom propertiesThe styles for different page formats contain significant repetition. Consider refactoring to use CSS custom properties to simplify the code and enhance maintainability.
Here's how you might refactor the code:
:root { --translation-group-height-portrait: 35%; --translation-group-height-landscape: 42%; --image-container-height-portrait: 62%; --image-container-height-landscape: 55%; --font-size-large: 600%; --font-size-medium: 400%; } /* Portrait orientations */ .pictureAndWordPage[class*="Portrait"] .bloom-translationGroup { height: var(--translation-group-height-portrait); } .pictureAndWordPage[class*="Portrait"] .bloom-imageContainer { height: var(--image-container-height-portrait); } /* Landscape orientations */ .pictureAndWordPage[class*="Landscape"] .bloom-translationGroup { height: var(--translation-group-height-landscape); } .pictureAndWordPage[class*="Landscape"] .bloom-imageContainer { height: var(--image-container-height-landscape); } /* Font sizes */ .pictureAndWordPage .bloom-content1 { font-size: var(--font-size-medium); } .pictureAndWordPage[class*="Landscape"] .bloom-content1 { font-size: var(--font-size-large); }This approach will:
- Improve maintainability by centralizing common values.
- Simplify adjustments to styles across multiple page formats.
- Reduce the potential for inconsistencies and errors.
.storybook/main.ts (1)
23-26: Consider improving the book ID mapping implementation.The hardcoded book ID mapping could be improved in several ways:
- Add TypeScript interface for better type safety
- Consider moving this to a separate configuration file
- Add comments explaining what these specific books are used for
Consider applying this improvement:
+interface BookMapping { + [key: string]: string; +} + +// Map of book IDs to their corresponding test book names +// multibook-index: Main index book for navigation testing +// multibook-target1: Target book for navigation testing const idToBookName: BookMapping = { "2e492eb1-bcc5-4b2b-b756-6cda33e1eee4": "multibook-index", "2c1b71ac-f399-446d-8398-e61a8efd4e83": "multibook-target1", };src/navigation.ts (1)
45-49: Simplify attribute access using optional chainingYou can streamline the code by utilizing optional chaining, which enhances readability.
Apply this diff:
const href: string = - (linkElement.attributes["href"] && - linkElement.attributes["href"].nodeValue) || - (linkElement.attributes["data-href"] && - linkElement.attributes["data-href"].nodeValue); + linkElement.attributes["href"]?.nodeValue || + linkElement.attributes["data-href"]?.nodeValue;🧰 Tools
🪛 Biome (1.9.4)
[error] 46-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
public/testBooks/multibook-target1/origami.css (1)
118-128: Remove commented code to clean up the stylesheetThere is commented-out code within the CSS that may no longer be needed. Consider removing it to improve maintainability.
Lines to remove:
/* JohnT mod: this extra padding makes it impossible to grab the crop control right above the split. padding-bottom: 5px; /* this actually enlarges the target area for the user trying to find the splitter, without changing the layout of the page (because the position is absolute)*/public/testBooks/multibook-index/appearance.css (1)
136-137: Remove duplicate.LegalLandscapeselectorThe
.LegalLandscapeselector appears twice, which is redundant and could cause confusion.Apply this diff:
.A4Portrait, .B5Portrait, .LetterLandscape, .LetterPortrait, -.LegalLandscape, .LegalLandscape {README-advanced.md (2)
28-28: Simplify phrasing for clarityConsider rephrasing the sentence to improve readability.
Change:
-`npm` so as to be readily available to a variety of clients. +`npm` to be readily available to clients.🧰 Tools
🪛 LanguageTool
[style] ~28-~28: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...is designed to be published usingnpmso as to be readily available to a variety of cl...(SO_AS_TO)
[style] ~28-~28: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...npmso as to be readily available to a variety of clients. The published version includes...(A_VARIETY_OF)
[grammar] ~28-~28: Probable usage error. Use “and” after ‘both’.
Context: ...cludes bothdist/bloomPlayer-HASH.js, as well as some mp3 files used by activities.W...
(BOTH_AS_WELL_AS)
28-28: Correct usage of 'both'When using 'both', it's standard to use 'and' rather than 'as well as'.
Apply this diff:
-The published version includes both `dist/bloomPlayer-HASH.js`, as well as some mp3 files used by activities. +The published version includes both `dist/bloomPlayer-HASH.js` and some mp3 files used by activities.🧰 Tools
🪛 LanguageTool
[style] ~28-~28: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...is designed to be published usingnpmso as to be readily available to a variety of cl...(SO_AS_TO)
[style] ~28-~28: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...npmso as to be readily available to a variety of clients. The published version includes...(A_VARIETY_OF)
[grammar] ~28-~28: Probable usage error. Use “and” after ‘both’.
Context: ...cludes bothdist/bloomPlayer-HASH.js, as well as some mp3 files used by activities.W...
(BOTH_AS_WELL_AS)
src/bloom-player-core.tsx (1)
2252-2254: Remove commented codeThe
fixInternalHyperlinksmethod is commented out but the code is still present. Since it's been replaced bycheckClickForBookOrPageJump, this commented code should be removed to maintain code cleanliness.public/testBooks/multibook-target1/appearance.css (2)
29-29: Consider using standard CSS values for visibility control.The variable
--pageNumber-showuses a non-standard valuedoShow-css-will-ignore-this-and-use-default. Consider using standard CSS values likeblock/nonefor better maintainability.- --pageNumber-show: doShow-css-will-ignore-this-and-use-default; /* default is number displaying (the value is a self-documenting trick) */ + --pageNumber-show: block; /* Use standard CSS value for better maintainability */
187-190: Remove commented-out code.The commented-out CSS rules should be removed if they're no longer needed. If they contain important information, consider moving it to documentation.
src/controlBar.tsx (3)
39-39: Remove unused import.The
Buttonimport from @material-ui/core is not used in the code.-import { Button } from "@material-ui/core";
Line range hint
270-296: Improve readability of conditional rendering.The nested ternary expressions make the code harder to read. Consider extracting this logic into a separate function that returns the appropriate component.
+ const getBackButtonIcon = (state: BackButtonState) => { + switch (state) { + case BackButtonState.showArrow: + return ( + <ArrowBack + aria-label="Go Back" + titleAccess={LocalizationManager.getTranslation( + "Button.Back", + props.preferredLanguages, + "Back" + )} + /> + ); + case BackButtonState.showEllipsis: + return ( + <MoreHoriz + aria-label="More Menu" + titleAccess={LocalizationManager.getTranslation( + "Button.More", + props.preferredLanguages, + "More" + )} + /> + ); + default: + return null; + } + }; - {props.getBackButtonState() === BackButtonState.showArrow ? ( - <ArrowBack - aria-label="Go Back" - titleAccess={LocalizationManager.getTranslation( - "Button.Back", - props.preferredLanguages, - "Back" - )} - /> - ) : ( - props.getBackButtonState() === BackButtonState.showEllipsis && ( - <MoreHoriz - aria-label="More Menu" - titleAccess={LocalizationManager.getTranslation( - "Button.More", - props.preferredLanguages, - "More" - )} - /> - ) - )} + {getBackButtonIcon(props.getBackButtonState())}
255-268: Simplify back button click handler.The click handler check can be combined with the disabled state check for more concise code.
<IconButton color="secondary" - onClick={() => { - if (props.backClicked) { - props.backClicked(); - } - }} + onClick={props.backClicked} data-testid="history-back-button" disabled={ - props.getBackButtonState() === BackButtonState.showNothing + !props.backClicked || props.getBackButtonState() === BackButtonState.showNothing } >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (16)
public/testBooks/multibook-index/BloomLocal.svgis excluded by!**/*.svgpublic/testBooks/multibook-index/back-cover-local.svgis excluded by!**/*.svgpublic/testBooks/multibook-index/happy-pirate.pngis excluded by!**/*.pngpublic/testBooks/multibook-index/history.dbis excluded by!**/*.dbpublic/testBooks/multibook-index/image.jpgis excluded by!**/*.jpgpublic/testBooks/multibook-index/license.pngis excluded by!**/*.pngpublic/testBooks/multibook-index/placeHolder.pngis excluded by!**/*.pngpublic/testBooks/multibook-index/thumbnail.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/BloomLocal.svgis excluded by!**/*.svgpublic/testBooks/multibook-target1/back-cover-local.svgis excluded by!**/*.svgpublic/testBooks/multibook-target1/history.dbis excluded by!**/*.dbpublic/testBooks/multibook-target1/image.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/license.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/placeHolder.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/thumbnail.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (45)
.storybook/main.ts(2 hunks).storybook/preview.ts(1 hunks)README-advanced.md(2 hunks)README.md(4 hunks)package.json(2 hunks)public/testBooks/multibook-index/Basic Book.css(1 hunks)public/testBooks/multibook-index/Traditional-XMatter.css(1 hunks)public/testBooks/multibook-index/appearance.css(1 hunks)public/testBooks/multibook-index/appearance.json(1 hunks)public/testBooks/multibook-index/basePage.css(1 hunks)public/testBooks/multibook-index/book.userPrefs(1 hunks)public/testBooks/multibook-index/branding.css(1 hunks)public/testBooks/multibook-index/customCollectionStyles.css(1 hunks)public/testBooks/multibook-index/defaultLangStyles.css(1 hunks)public/testBooks/multibook-index/index.htm(1 hunks)public/testBooks/multibook-index/meta.json(1 hunks)public/testBooks/multibook-index/origami.css(1 hunks)public/testBooks/multibook-index/previewMode.css(1 hunks)public/testBooks/multibook-index/publish-settings.json(1 hunks)public/testBooks/multibook-target1/Basic Book.css(1 hunks)public/testBooks/multibook-target1/Traditional-XMatter.css(1 hunks)public/testBooks/multibook-target1/appearance.css(1 hunks)public/testBooks/multibook-target1/appearance.json(1 hunks)public/testBooks/multibook-target1/basePage.css(1 hunks)public/testBooks/multibook-target1/book.userPrefs(1 hunks)public/testBooks/multibook-target1/branding.css(1 hunks)public/testBooks/multibook-target1/customCollectionStyles.css(1 hunks)public/testBooks/multibook-target1/defaultLangStyles.css(1 hunks)public/testBooks/multibook-target1/index.htm(1 hunks)public/testBooks/multibook-target1/meta.json(1 hunks)public/testBooks/multibook-target1/origami.css(1 hunks)public/testBooks/multibook-target1/previewMode.css(1 hunks)public/testBooks/multibook-target1/publish-settings.json(1 hunks)src/bloom-player-controls.tsx(4 hunks)src/bloom-player-core.tsx(25 hunks)src/bloomPlayerAnalytics.tsx(1 hunks)src/bookInfo.ts(1 hunks)src/controlBar.tsx(5 hunks)src/externalContext.ts(1 hunks)src/navigation.test.ts(1 hunks)src/navigation.ts(1 hunks)src/stories/BloomPlayerIframe.tsx(1 hunks)src/stories/BloomPlayerTester.ts(1 hunks)src/stories/multiBook.stories.tsx(1 hunks)src/stories/navigation.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (28)
- public/testBooks/multibook-target1/customCollectionStyles.css
- .storybook/preview.ts
- src/bloomPlayerAnalytics.tsx
- public/testBooks/multibook-index/customCollectionStyles.css
- src/externalContext.ts
- public/testBooks/multibook-index/defaultLangStyles.css
- src/bookInfo.ts
- public/testBooks/multibook-index/appearance.json
- public/testBooks/multibook-index/Traditional-XMatter.css
- public/testBooks/multibook-target1/book.userPrefs
- public/testBooks/multibook-target1/branding.css
- public/testBooks/multibook-target1/appearance.json
- public/testBooks/multibook-index/publish-settings.json
- src/stories/BloomPlayerIframe.tsx
- public/testBooks/multibook-index/meta.json
- public/testBooks/multibook-target1/defaultLangStyles.css
- src/stories/navigation.stories.tsx
- public/testBooks/multibook-index/origami.css
- public/testBooks/multibook-index/branding.css
- public/testBooks/multibook-index/book.userPrefs
- public/testBooks/multibook-target1/meta.json
- public/testBooks/multibook-index/previewMode.css
- src/navigation.test.ts
- src/stories/multiBook.stories.tsx
- public/testBooks/multibook-target1/publish-settings.json
- package.json
- public/testBooks/multibook-target1/index.htm
- public/testBooks/multibook-index/index.htm
🧰 Additional context used
🪛 Biome (1.9.4)
src/bloom-player-core.tsx
[error] 1894-1894: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/navigation.ts
[error] 44-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
README-advanced.md
[style] ~28-~28: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...is designed to be published using npm so as to be readily available to a variety of cl...
(SO_AS_TO)
[style] ~28-~28: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ... npm so as to be readily available to a variety of clients. The published version includes...
(A_VARIETY_OF)
[grammar] ~28-~28: Probable usage error. Use “and” after ‘both’.
Context: ...cludes both dist/bloomPlayer-HASH.js, as well as some mp3 files used by activities.
W...
(BOTH_AS_WELL_AS)
README.md
[uncategorized] ~57-~57: Consider adding a hyphen.
Context: ...tton If true, displays a button in the upper left corner of the app bar. When clicked, bl...
(TOP_LEFT_CORNER)
[grammar] ~57-~57: A determiner cannot be combined with a possessive pronoun. Did you mean simply “the” or “your”?
Context: ...post a message "backButtonClicked" that the your host can catch and respond to. Example...
(A_MY)
[uncategorized] ~64-~64: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...mally this button will be a left arrow. However if bloom-player is the top level window...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~64-~64: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t arrow. However if bloom-player is the top level window (i.e. it is not embedded in anot...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[formatting] ~106-~106: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...r. However, Bloom Player needs your help, because it does not have a way to know what the...
(COMMA_BEFORE_BECAUSE)
🪛 Markdownlint (0.35.0)
README.md
109-109: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
🔇 Additional comments (28)
public/testBooks/multibook-target1/Traditional-XMatter.css (5)
1-9: LGTM! Good use of pseudo-elements and accessibility considerations.
The implementation properly handles page labels using pseudo-elements while maintaining accessibility by keeping the "Traditional" text in the DOM.
52-53: Fix invalid CSS value kFrontCoverImageOrder
The value kFrontCoverImageOrder appears to be intended as a CSS variable but is used directly. This will cause the order property to be invalid.
Also applies to: 58-59
111-226: LGTM! Well-structured layout with good responsive design practices.
The title page implementation demonstrates good practices:
- Proper use of flexbox for layout management
- Responsive design with flexible gaps
- CSS variables for configuration
- Appropriate image handling with object-fit
227-243: LGTM! Clean and straightforward implementation.
The back cover styling is appropriately minimal while maintaining consistency with the overall layout approach.
250-255: Consider making ISBN dimensions more flexible
The hard-coded dimensions and use of !important might cause maintainability issues.
public/testBooks/multibook-target1/previewMode.css (1)
33-35: Reconsider global cursor restriction
Setting cursor: not-allowed globally on the html element might be too restrictive and could affect interactive elements that should remain clickable in preview mode.
public/testBooks/multibook-target1/basePage.css (2)
1-14: LGTM: Base styles are well-structured
The base typography and layout styles are well-defined with:
- Appropriate font stack with fallbacks
- Good contrast for audio highlights
- Proper flex layout for text containers
907-918: Avoid using unsupported CSS pseudo-class :has()
The CSS uses the :has() pseudo-class selector, which may cause compatibility issues.
public/testBooks/multibook-target1/Basic Book.css (2)
164-347: Reduce code duplication using CSS custom properties
As mentioned in earlier reviews, the styles for different page formats contain significant repetition. Refactoring to use CSS custom properties would enhance code maintainability.
Refer to the suggested refactoring approach provided previously.
65-87:
Fix typo in class name 'nospecidalmovement'
As previously noted, there is a typo in the class name 'nospecidalmovement_imageOnTop'; it should be 'nospecialmovement_imageOnTop'.
Apply this correction throughout the file:
-.nospecidalmovement_imageOnTop
+.nospecialmovement_imageOnTop.storybook/main.ts (4)
38-50: Handle undefined bookName to prevent potential errors.
The bookName could be undefined if the bookId is not found in the mapping.
61-65: Fix potential override of addons configuration.
Setting addons: ["@storybook/addon-interactions"] overwrites the existing addons array.
10-12: LGTM! Verify the build step documentation.
The static directory configuration and documentation are clear. The comment helpfully explains the build requirement for iframe-related stories.
Let's verify if the build step is documented in the repository:
✅ Verification successful
Build step is properly documented in package.json
The buildForStorybook script is well documented in package.json with a clear comment explaining its purpose: "run this when working with stories that use an iframe and bloomplayer.htm instead of the raw react components". This aligns perfectly with the comment in .storybook/main.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the build step is documented in README or package.json
echo "Checking README for build documentation..."
rg -i "buildForStorybook|build.*storybook" README.md
echo "Checking package.json for build script..."
if [ -f "package.json" ]; then
jq '.scripts | select(. | to_entries[] | .key | contains("buildForStorybook"))' package.json
fi
Length of output: 1527
53-59:
Review the security implications of using file:/// protocol.
Using the file:/// protocol in the proxy target could potentially expose local files. Consider:
- Adding path validation
- Restricting access to specific directories
- Using a more secure proxy method
Let's check for other file protocol usage in the codebase:
src/navigation.ts (2)
33-37: 🛠️ Refactor suggestion
Enhance type safety and null checks in event handling
The event parameter is typed as any. Consider specifying it as MouseEvent and adding null checks to improve type safety.
Apply this diff:
-export function checkClickForBookOrPageJump(
- event: any,
+export function checkClickForBookOrPageJump(
+ event: MouseEvent,
currentBookInstanceId: string,
getCurentPageId: () => string,
): ILocation {
- const linkElement = (event.target as HTMLElement).closest(
+ const target = event.target as HTMLElement;
+ if (!target) return {};
+ const linkElement = target.closest(
"[href], [data-href]",
);106-112:
Add error handling for URL parsing
The parseTargetBookUrl function may throw an exception if the URL is malformed. Consider adding error handling to manage such cases gracefully.
Apply this diff:
function parseTargetBookUrl(url: string) {
// the format is "/book/BOOKID#PAGEID" where the page id is optional
+ try {
const bloomUrl = new URL(url, window.location.origin);
const bookId = bloomUrl.pathname.replace("/book/", "");
const pageId = bloomUrl.hash.replace("#", "");
return { bookId, pageId };
+ } catch (error) {
+ console.error("Error parsing URL:", error);
+ return { bookId: "", pageId: "" };
+ }
}src/stories/BloomPlayerTester.ts (3)
115-118: Replace pause function with a parameterized sleep utility
Create a reusable sleep function that accepts a duration parameter to improve code flexibility.
Apply this diff:
-/* oh, the shame! */
-async function pause() {
- await new Promise((resolve) => setTimeout(resolve, 300));
-}
+/**
+ * Pauses execution for the specified duration.
+ * @param ms The duration to pause in milliseconds.
+ */
+function sleep(ms: number) {
+ return new Promise((resolve) => setTimeout(resolve, ms));
+}Update calls to pause() to use sleep(ms) with appropriate durations.
3-7: 🛠️ Refactor suggestion
Improve type safety by handling iframeDoc as Document | null
Currently, iframeDoc is initialized using null as unknown as Document, which is not type-safe. Change the type to Document | null and handle null cases appropriately.
Apply this diff:
- private iframeDoc: Document;
+ private iframeDoc: Document | null = null;
- this.iframeDoc = null as unknown as Document;Update usages of iframeDoc to handle possible null values.
22-25:
Fix potential race condition when attaching onload handler
There's a risk that the iframe loads before the onload handler is attached, causing the promise to hang. Adjust the code to prevent this issue.
Apply this diff:
this.iframeDoc = await new Promise<Document>((resolve) => {
+ const onLoadHandler = () => resolve(iframe.contentDocument!);
+ iframe.addEventListener('load', onLoadHandler);
+ if (iframe.contentDocument) {
+ iframe.removeEventListener('load', onLoadHandler);
+ resolve(iframe.contentDocument);
+ }
- iframe.onload = () => resolve(iframe.contentDocument!);
});README.md (3)
5-11: LGTM!
The list of Bloom project usage is clear and well-documented.
102-130: Well-documented book linking implementation!
The documentation clearly explains:
- How books link using Instance IDs
- Resource request handling process
- Implementation steps for hosts
🧰 Tools
🪛 LanguageTool
[formatting] ~106-~106: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...r. However, Bloom Player needs your help, because it does not have a way to know what the...
(COMMA_BEFORE_BECAUSE)
🪛 Markdownlint (0.35.0)
109-109: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
151-154: LGTM!
Testing instructions are clear and easy to follow.
src/bloom-player-controls.tsx (2)
913-928: LGTM: Back button functionality is well-implemented
The back button implementation follows a clear hierarchy:
- First tries internal history navigation via
CanGoBack() - Falls back to host navigation if internal history is empty
- Intelligently handles different contexts (top window vs iframe)
Also applies to: 934-938
1041-1041: 🛠️ Refactor suggestion
Update property name for consistency
The property startPage is used here but has been renamed to startPageIndex in the interface. This should be updated for consistency.
- startPageIndex={props.startPage}
+ startPageIndex={props.startPageIndex}Likely invalid or redundant comment.
public/testBooks/multibook-index/basePage.css (1)
1-1304: LGTM: Well-structured CSS implementation
The CSS file provides comprehensive styling with:
- Clear organization of rules by component
- Proper use of CSS variables for maintainability
- Responsive design handling for different page sizes
- Proper fallbacks and browser compatibility
src/bloom-player-core.tsx (2)
361-370: LGTM: Improved navigation handling
The new click handler with checkClickForBookOrPageJump provides a more robust navigation system:
- Properly handles book and page jumps
- Maintains navigation history
- Efficiently retrieves page IDs only when needed
1732-1832: LGTM: Robust font handling implementation
The font handling implementation is well-designed with:
- Proper fallback chain: local → host → web fonts
- Support for all font variants (regular, bold, italic)
- Performance optimization by trying local fonts first
- Clear documentation of the font loading strategy
public/testBooks/multibook-target1/appearance.css (1)
129-140:
Remove duplicate .LegalLandscape class.
The selector list contains a duplicate entry for .LegalLandscape.
.A3Landscape,
.A3Portrait,
.A4Landscape,
.A4Portrait,
.B5Portrait,
.LetterLandscape,
.LetterPortrait,
.LegalLandscape,
-.LegalLandscape {
+.LegalPortrait {
--page-margin: 15mm;
--page-gutter: 10mm;
}| .nospecidalmovement_imageOnTop .bloom-imageContainer { | ||
| height: calc(45% - var(--multilingual-editable-vertical-gap)); | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| top: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-translationGroup { | ||
| height: 50%; | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| bottom: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-editable.bloom-monolingual { | ||
| height: 100%; | ||
| } | ||
| .bloom-bilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(50% - var(--multilingual-editable-vertical-gap)); | ||
| } | ||
| .bloom-trilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(33% - var(--multilingual-editable-vertical-gap)); | ||
| } |
There was a problem hiding this comment.
Fix typo in class name 'nospecidalmovement'
There appears to be a typo in the class name 'nospecidalmovement_imageOnTop'; it should be 'nospecialmovement_imageOnTop'.
Apply this correction throughout the file:
-.nospecidalmovement_imageOnTop
+.nospecialmovement_imageOnTop📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .nospecidalmovement_imageOnTop .bloom-imageContainer { | |
| height: calc(45% - var(--multilingual-editable-vertical-gap)); | |
| width: 100%; | |
| position: absolute; | |
| left: 0; | |
| top: 0; | |
| } | |
| .nospecidalmovement_imageOnTop .bloom-translationGroup { | |
| height: 50%; | |
| width: 100%; | |
| position: absolute; | |
| left: 0; | |
| bottom: 0; | |
| } | |
| .nospecidalmovement_imageOnTop .bloom-editable.bloom-monolingual { | |
| height: 100%; | |
| } | |
| .bloom-bilingual.nospecidalmovement_imageOnTop .bloom-editable { | |
| height: calc(50% - var(--multilingual-editable-vertical-gap)); | |
| } | |
| .bloom-trilingual.nospecidalmovement_imageOnTop .bloom-editable { | |
| height: calc(33% - var(--multilingual-editable-vertical-gap)); | |
| } | |
| .nospecialmovement_imageOnTop .bloom-imageContainer { | |
| height: calc(45% - var(--multilingual-editable-vertical-gap)); | |
| width: 100%; | |
| position: absolute; | |
| left: 0; | |
| top: 0; | |
| } | |
| .nospecialmovement_imageOnTop .bloom-translationGroup { | |
| height: 50%; | |
| width: 100%; | |
| position: absolute; | |
| left: 0; | |
| bottom: 0; | |
| } | |
| .nospecialmovement_imageOnTop .bloom-editable.bloom-monolingual { | |
| height: 100%; | |
| } | |
| .bloom-bilingual.nospecialmovement_imageOnTop .bloom-editable { | |
| height: calc(50% - var(--multilingual-editable-vertical-gap)); | |
| } | |
| .bloom-trilingual.nospecialmovement_imageOnTop .bloom-editable { | |
| height: calc(33% - var(--multilingual-editable-vertical-gap)); | |
| } |
There was a problem hiding this comment.
@hatton, understood. Skipping this.
There was a problem hiding this comment.
Rate Limit Exceeded
@hatton have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 53 seconds before sending another message.
There was a problem hiding this comment.
Reviewed 51 of 62 files at r1, all commit messages.
Reviewable status: 51 of 62 files reviewed, 30 unresolved discussions (waiting on @coderabbitai[bot] and @hatton)
public/testBooks/multibook-index/index.htm line 1 at r2 (raw file):
<!DOCTYPE html>
Note that I did not really review the files in public/testBooks/multibook-index{1} but there are certainly some we could remove such as
- history.db
- userprefs
- thumbnail
- distribution
- publish-settings
src/externalContext.ts line 116 at r2 (raw file):
let gotCapabilities = false; export function sendBackToHost() {
There is an ambiguity in this name between
- send to the host the fact that the client wants to perform a back
- send the user back to the host
Indeed, until I saw more of the changes, the first time I saw this name elsewhere, I thought it was the latter.
Ideas:
- performBackAction
- informHostOfBackAction
.storybook/main.ts line 11 at r2 (raw file):
stories: ["../src/**/*.stories.@(js|jsx|mjs|ts|tsx)"], // For stories that include an iframe and thus need access to bloomplayer.htm and the bundles it loads. // When using those stories, use `yarn buildForStorybook` to update non-storybook code.
Should there be a reference to this in the README?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
.storybook/main.ts (2)
23-26: Consider improving type safety and maintainabilityThe hardcoded book ID mapping could benefit from:
- TypeScript interface/type definition
- Moving to a separate configuration file for easier maintenance
Consider applying this improvement:
+interface BookMapping { + [key: string]: string; +} + +// Consider moving this to a separate config file +const idToBookName: BookMapping = { -const idToBookName = { "2e492eb1-bcc5-4b2b-b756-6cda33e1eee4": "multibook-index", "2c1b71ac-f399-446d-8398-e61a8efd4e83": "multibook-target1", };
27-60: Consider enhancing error handling and monitoringThe proxy configuration handles critical path routing for book resources. Consider adding:
- Structured error logging for failed rewrites
- Telemetry for tracking proxy performance
- Health checks for the proxy endpoints
This will help with debugging and maintaining the system in production.
README.md (4)
57-57: Fix grammatical errors and improve clarity in 'showBackButton' descriptionThere are minor grammatical errors in the description. Consider making the following changes to improve clarity.
Apply this diff to fix the issues:
-If true, displays a button in the upper left corner of the app bar. When clicked, bloom-player will post a message "backButtonClicked" that the your host can catch and respond to. +If true, displays a button in the upper-left corner of the app bar. When clicked, bloom-player will post a message "backButtonClicked" that your host can catch and respond to.🧰 Tools
🪛 LanguageTool
[uncategorized] ~57-~57: Consider adding a hyphen.
Context: ...tton If true, displays a button in the upper left corner of the app bar. When clicked, bl...(TOP_LEFT_CORNER)
[grammar] ~57-~57: A determiner cannot be combined with a possessive pronoun. Did you mean simply “the” or “your”?
Context: ...post a message "backButtonClicked" that the your host can catch and respond to. Example...(A_MY)
64-64: Add missing comma and hyphenate compound adjectiveAdd a comma after 'However' and hyphenate 'top level' to form 'top-level' in 'top-level window'.
Apply this diff to correct the sentence:
-> Normally this button will be a left arrow. However if bloom-player is the top level window (i.e. it is not embedded in another page), it will show as an ellipsis. +> Normally this button will be a left arrow. However, if bloom-player is the top-level window (i.e., it is not embedded in another page), it will show as an ellipsis.🧰 Tools
🪛 LanguageTool
[uncategorized] ~64-~64: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...mally this button will be a left arrow. However if bloom-player is the top level window...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~64-~64: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t arrow. However if bloom-player is the top level window (i.e. it is not embedded in anot...(EN_COMPOUND_ADJECTIVE_INTERNAL)
78-78: Hyphenate compound adjectives in 'hideFullScreenButton' descriptionTo improve readability, hyphenate compound adjectives such as 'Full Screen' and 'upper right-hand corner'.
Apply this diff:
-If true, the Full Screen icon button will not appear in the upper right hand corner of the window. Otherwise, a Full Screen icon button is displayed and allows the user to toggle between full screen and window mode. +If true, the Full-Screen icon button will not appear in the upper right-hand corner of the window. Otherwise, a Full-Screen icon button is displayed and allows the user to toggle between full-screen and window mode.🧰 Tools
🪛 LanguageTool
[uncategorized] ~78-~78: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...#### hideFullScreenButton If true, the Full Screen icon button will not appear in the uppe...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~78-~78: Did you mean the adjective “right-hand”?
Context: ...con button will not appear in the upper right hand corner of the window. Otherwise, a Full...(MISSING_HYPHEN)
[uncategorized] ~78-~78: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...hand corner of the window. Otherwise, a Full Screen icon button is displayed and allows the...(EN_COMPOUND_ADJECTIVE_INTERNAL)
161-161: Add missing comma in 'Running unit tests' sectionAdd a comma after 'tests' to improve readability.
Apply this diff:
-To run unit tests use `yarn test`. This will run all `*.test.ts`, which should be collocated with the thing being tested. +To run unit tests, use `yarn test`. This will run all `*.test.ts`, which should be collocated with the thing being tested.🧰 Tools
🪛 LanguageTool
[uncategorized] ~161-~161: Possible missing comma found.
Context: ...y. ### Running unit tests To run unit tests useyarn test. This will run all `*.t...(AI_HYDRA_LEO_MISSING_COMMA)
src/bloom-player-controls.tsx (1)
76-76: Fix typo in commentThere's a typo in the comment: 'us a proxy' should be 'use a proxy'.
Apply this diff:
-// TODO can we get rid of this and us a proxy? See .storybook/main.ts +// TODO can we get rid of this and use a proxy? See .storybook/main.tssrc/bloom-player-core.tsx (1)
1908-1909: Simplify null checks using optional chainingYou can simplify the null check by using optional chaining.
Apply this diff:
-if (result && result.data) { +if (result?.data) {🧰 Tools
🪛 Biome (1.9.4)
[error] 1909-1909: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
.storybook/main.ts(2 hunks)README.md(6 hunks)package.json(2 hunks)src/bloom-player-controls.tsx(5 hunks)src/bloom-player-core.tsx(25 hunks)src/navigation.test.ts(1 hunks)src/navigation.ts(1 hunks)src/stories/BloomPlayerIframe.tsx(1 hunks)src/stories/multiBook.stories.tsx(1 hunks)src/stories/navigation.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/stories/navigation.stories.tsx
- src/navigation.test.ts
- src/stories/BloomPlayerIframe.tsx
- src/stories/multiBook.stories.tsx
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/bloom-player-core.tsx
[error] 1909-1909: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
README.md
[uncategorized] ~57-~57: Consider adding a hyphen.
Context: ...tton If true, displays a button in the upper left corner of the app bar. When clicked, bl...
(TOP_LEFT_CORNER)
[grammar] ~57-~57: A determiner cannot be combined with a possessive pronoun. Did you mean simply “the” or “your”?
Context: ...post a message "backButtonClicked" that the your host can catch and respond to. Example...
(A_MY)
[uncategorized] ~64-~64: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...mally this button will be a left arrow. However if bloom-player is the top level window...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~64-~64: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t arrow. However if bloom-player is the top level window (i.e. it is not embedded in anot...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~78-~78: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...#### hideFullScreenButton If true, the Full Screen icon button will not appear in the uppe...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~78-~78: Did you mean the adjective “right-hand”?
Context: ...con button will not appear in the upper right hand corner of the window. Otherwise, a Full...
(MISSING_HYPHEN)
[uncategorized] ~78-~78: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...hand corner of the window. Otherwise, a Full Screen icon button is displayed and allows the...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[formatting] ~106-~106: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...r. However, Bloom Player needs your help, because it does not have a way to know what the...
(COMMA_BEFORE_BECAUSE)
[uncategorized] ~161-~161: Possible missing comma found.
Context: ...y. ### Running unit tests To run unit tests use yarn test. This will run all `*.t...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint (0.35.0)
README.md
110-110: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
🔇 Additional comments (7)
.storybook/main.ts (4)
38-50: Handle undefined bookName to prevent potential errors
This is a previously identified issue that still needs addressing.
61-65: Fix potential override of addons configuration
This is a previously identified issue that still needs addressing.
10-12: LGTM! Verify the dist directory setup
The static directory configuration is correct and well-documented. The comments provide clear guidance about using yarn watchForStorybook.
✅ Verification successful
The staticDirs: ["../dist"] configuration is correct and well-supported
The setup is verified to be correct because:
- The
distdirectory is a build output directory configured invite.config.ts - The
watchForStorybookscript inpackage.jsonspecifically builds and watches this directory for development - Multiple dist artifacts (JS, HTML, MP3, CSS, TSV) are referenced in the package.json
- The comment's instruction to use
yarn watchForStorybookaligns with the available npm script
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the dist directory and its contents
fd -t d "^dist$" -x ls -la {} \;
Length of output: 37
Script:
#!/bin/bash
# Let's check if there are any references to the dist directory in package.json or other configuration files
rg "dist" -g "package.json" -g "*.config.*" -g ".storybook/*"
# Also check for any build or watch scripts that might generate the dist directory
rg "watchForStorybook|build" package.json
Length of output: 587
28-30: Clarify CORS resolution strategy
The comment indicates that CORS issues are not resolved ("THIS IS NOT WORKING"). This needs to be addressed or documented more clearly.
src/navigation.ts (2)
107-113: Add error handling for URL parsing
The parseTargetBookUrl function should handle potential exceptions when parsing the URL to prevent the application from crashing if an invalid URL is passed.
28-32: Improve type safety and add null checks in event handling
The event parameter should be typed more specifically than any, such as MouseEvent. Additionally, ensure that null checks are in place when accessing event properties to prevent runtime errors.
src/bloom-player-controls.tsx (1)
1041-1041:
Fix incorrect prop usage: use props.startPageIndex instead of props.startPage
In the BloomPlayerCore component, the startPageIndex prop is being set to props.startPage, which is outdated. It should use props.startPageIndex to match the updated prop name in BloomPlayerProps.
Apply this diff to fix the issue:
-startPageIndex={props.startPage}
+startPageIndex={props.startPageIndex}| if (!bloomPage) { | ||
| throw new Error("No bloomPage at index " + index); | ||
| } | ||
| return bloomPage.getAttribute("id")!; |
There was a problem hiding this comment.
Handle potential null value when accessing page ID
The id attribute may be null. Consider adding error handling to avoid potential runtime errors.
Apply this diff:
-return bloomPage.getAttribute("id")!;
+const pageId = bloomPage.getAttribute("id");
+if (!pageId) {
+ throw new Error("No page ID found for index " + index);
+}
+return pageId;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return bloomPage.getAttribute("id")!; | |
| const pageId = bloomPage.getAttribute("id"); | |
| if (!pageId) { | |
| throw new Error("No page ID found for index " + index); | |
| } | |
| return pageId; |
|
I think all this got readded by accident during a merge? |
| const bookId = path.split("/")[2]; | ||
| const bookName = idToBookName[bookId]; | ||
| const rewrittenPath = path.replace( | ||
| `/book/${bookId}`, | ||
| `testBooks/${bookName}`, | ||
| ); |
| return { | ||
| ...config, | ||
| server: { ...config.server, proxy: updatedProxy }, | ||
| addons: ["@storybook/addon-interactions"], | ||
| }; |
| .A3Landscape, | ||
| .A3Portrait, | ||
| .A4Landscape, | ||
| .A4Portrait, | ||
| .B5Portrait, | ||
| .LetterLandscape, | ||
| .LetterPortrait, | ||
| .LegalLandscape, | ||
| .LegalLandscape { | ||
| --page-margin: 15mm; | ||
| --page-gutter: 10mm; | ||
| } |
| .nospecidalmovement_imageOnTop .bloom-imageContainer { | ||
| height: calc(45% - var(--multilingual-editable-vertical-gap)); | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| top: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-translationGroup { | ||
| height: 50%; | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| bottom: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-editable.bloom-monolingual { | ||
| height: 100%; | ||
| } | ||
| .bloom-bilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(50% - var(--multilingual-editable-vertical-gap)); | ||
| } | ||
| .bloom-trilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(33% - var(--multilingual-editable-vertical-gap)); | ||
| } |
| /* these rules allow different branding insertions depending on orientation */ | ||
| .portraitOnly, | ||
| .landscapeOnly { | ||
| display: none; | ||
| } | ||
| .Size6x9Portrait .portraitOnly, | ||
| .USComicPortrait .portraitOnly, | ||
| .Device16x9Portrait .portraitOnly, | ||
| .QuarterLetterPortrait .portraitOnly, | ||
| .HalfLetterPortrait .portraitOnly, | ||
| .LetterPortrait .portraitOnly, | ||
| .LegalPortrait .portraitOnly, | ||
| .A3Portrait .portraitOnly, | ||
| .A4Portrait .portraitOnly, | ||
| .A5Portrait .portraitOnly, | ||
| .B5Portrait .portraitOnly, | ||
| .A6Portrait .portraitOnly { | ||
| display: unset; | ||
| } | ||
| .Size6x9Landscape .landscapeOnly, | ||
| .Cm13Landscape .landscapeOnly, | ||
| .Device16x9Landscape .landscapeOnly, | ||
| .LetterLandscape .landscapeOnly, | ||
| .LegalLandscape .landscapeOnly, | ||
| .A3Landscape .landscapeOnly, | ||
| .A4Landscape .landscapeOnly, | ||
| .A5Landscape .landscapeOnly, | ||
| .A6Landscape .landscapeOnly { | ||
| display: unset; | ||
| } |
| .outsideBackCover img.branding { | ||
| margin-top: 4px; | ||
| width: 10cm; | ||
| } |
There was a problem hiding this comment.
Reviewable status: 48 of 62 files reviewed, 32 unresolved discussions (waiting on @andrew-polk and @coderabbitai[bot])
.storybook/main.ts line 11 at r2 (raw file):
Previously, andrew-polk wrote…
Should there be a reference to this in the README?
Personally I consider package.json to be the index to these things in itself. No value is added (but risk increased) by documenting in two places.
public/testBooks/multibook-index/index.htm line 1 at r2 (raw file):
Previously, andrew-polk wrote…
Note that I did not really review the files in
public/testBooks/multibook-index{1}but there are certainly some we could remove such as
- history.db
- userprefs
- thumbnail
- distribution
- publish-settings
done except distribution, which bloom-player reads
|
|
||
| <div class="bloom-editable bloom-nodefaultstylerule Title-On-Cover-style bloom-padForOverflow bloom-visibility-code-on bloom-content1 bloom-contentNational1 bloom-contentFirst" | ||
| lang="en" contenteditable="true" data-book="bookTitle" style="padding-bottom: 0px;" tabindex="0" | ||
| spellcheck="false" role="textbox" aria-label="false" data-hasqtip="true" aria-describedby="qtip-0"> |
| </div> | ||
|
|
||
| <div class="bloom-imageContainer"> | ||
| <img data-book="coverImage" src="placeHolder.png" alt=""></img> |
| <p><a href="#cover">This book Cover</a></p> | ||
|
|
||
| <p><a href="#0b22a586-3324-4a76-b95d-e49ca03105d8">This book pirate page</a></p> | ||
|
|
||
| <p><a href="/book/2c1b71ac-f399-446d-8398-e61a8efd4e83">Book 1</a> | ||
| </p> | ||
| <p><a href="/book/2c1b71ac-f399-446d-8398-e61a8efd4e83#b0f5224f-7a31-4944-84fb-f2cd46d56fc8">Book | ||
| 1 Crab Page</a> | ||
| </p> |
| .A3Landscape, | ||
| .A3Portrait, | ||
| .A4Landscape, | ||
| .A4Portrait, | ||
| .B5Portrait, | ||
| .LetterLandscape, | ||
| .LetterPortrait, | ||
| .LegalLandscape, | ||
| .LegalLandscape { | ||
| --page-margin: 15mm; | ||
| --page-gutter: 10mm; | ||
| } |
There was a problem hiding this comment.
Reviewable status: 48 of 62 files reviewed, 32 unresolved discussions (waiting on @andrew-polk and @coderabbitai[bot])
| .nospecidalmovement_imageOnTop .bloom-imageContainer { | ||
| height: calc(45% - var(--multilingual-editable-vertical-gap)); | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| top: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-translationGroup { | ||
| height: 50%; | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| bottom: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-editable.bloom-monolingual { | ||
| height: 100%; | ||
| } | ||
| .bloom-bilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(50% - var(--multilingual-editable-vertical-gap)); | ||
| } | ||
| .bloom-trilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(33% - var(--multilingual-editable-vertical-gap)); | ||
| } |
| .pictureAndWordPage.QuarterLetterLandscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.QuarterLetterLandscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.HalfLetterPortrait .bloom-translationGroup { | ||
| height: 35%; | ||
| } | ||
| .pictureAndWordPage.HalfLetterPortrait .bloom-imageContainer { | ||
| height: 62%; | ||
| } | ||
| .pictureAndWordPage.HalfLetterPortrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.LetterPortrait .bloom-translationGroup { | ||
| height: 23%; | ||
| } | ||
| .pictureAndWordPage.LetterPortrait .bloom-imageContainer { | ||
| height: 74%; | ||
| } | ||
| .pictureAndWordPage.LetterPortrait .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.LetterLandscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.LetterLandscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.LetterLandscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.LegalPortrait .bloom-translationGroup { | ||
| height: 23%; | ||
| } | ||
| .pictureAndWordPage.LegalPortrait .bloom-imageContainer { | ||
| height: 74%; | ||
| } | ||
| .pictureAndWordPage.LegalPortrait .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.LegalLandscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.LegalLandscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.LegalLandscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.Cm13Landscape .bloom-translationGroup { | ||
| height: 37%; | ||
| } | ||
| .pictureAndWordPage.Cm13Landscape .bloom-imageContainer { | ||
| height: 60%; | ||
| } | ||
| .pictureAndWordPage.Cm13Landscape .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.USComicPortrait .bloom-translationGroup { | ||
| height: 32%; | ||
| } | ||
| .pictureAndWordPage.USComicPortrait .bloom-imageContainer { | ||
| height: 65%; | ||
| } | ||
| .pictureAndWordPage.USComicPortrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Portrait .bloom-translationGroup { | ||
| height: 35%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Portrait .bloom-imageContainer { | ||
| height: 62%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Portrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Landscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Landscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Landscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage .bloom-translationGroup { | ||
| width: 100%; | ||
| text-align: center; | ||
| padding-top: 10px; | ||
| } | ||
| .pictureAndWordPage .bloom-editable { | ||
| vertical-align: middle; | ||
| height: auto; | ||
| } | ||
| .pictureAndWordPage .bloom-content2, | ||
| .pictureAndWordPage .bloom-content3 { | ||
| font-size: 200%; | ||
| } |
There was a problem hiding this comment.
we don't want to review html or css in this pr
| .bloom-vertical-align-center:not(:has(.overflow)) { | ||
| justify-content: center; | ||
| } | ||
| .bloom-vertical-align-center:not(:has(.overflow)):not(.bloom-bilingual):not(.bloom-trilingual) .bloom-editable { | ||
| justify-content: center; | ||
| } | ||
| .bloom-vertical-align-bottom:not(:has(.overflow)) { | ||
| justify-content: flex-end; | ||
| } | ||
| .bloom-vertical-align-bottom:not(:has(.overflow)):not(.bloom-bilingual):not(.bloom-trilingual) .bloom-editable { | ||
| justify-content: flex-end; | ||
| } |
There was a problem hiding this comment.
we don't want to review html or css in this pr
| .outsideBackCover img.branding { | ||
| margin-top: 4px; | ||
| width: 10cm; | ||
| } |
There was a problem hiding this comment.
we don't want to review html or css in this pr
| title=""><img src="image.png" alt="" data-copyright="" data-creator="" | ||
| data-license=""></img></div> |
There was a problem hiding this comment.
we don't want to review html or css in this pr
| html { | ||
| cursor: not-allowed; | ||
| } |
There was a problem hiding this comment.
we don't want to review html or css in this pr
| order: kFrontCoverImageOrder; | ||
| flex: 1 1 auto; |
There was a problem hiding this comment.
we don't want to review html or css in this pr
| .ISBNContainer .bloom-editable { | ||
| text-align: left !important; | ||
| width: 4.3cm !important; | ||
| max-height: 1.8em; | ||
| min-height: 1.8em; | ||
| } |
There was a problem hiding this comment.
we don't want to review html or css in this pr
There was a problem hiding this comment.
Reviewable status: 48 of 62 files reviewed, 32 unresolved discussions (waiting on @andrew-polk and @coderabbitai[bot])
| const bookId = path.split("/")[2]; | ||
| const bookName = idToBookName[bookId]; | ||
| const rewrittenPath = path.replace( | ||
| `/book/${bookId}`, | ||
| `testBooks/${bookName}`, | ||
| ); |
| return { | ||
| ...config, | ||
| server: { ...config.server, proxy: updatedProxy }, | ||
| addons: ["@storybook/addon-interactions"], | ||
| }; |
| .A3Landscape, | ||
| .A3Portrait, | ||
| .A4Landscape, | ||
| .A4Portrait, | ||
| .B5Portrait, | ||
| .LetterLandscape, | ||
| .LetterPortrait, | ||
| .LegalLandscape, | ||
| .LegalLandscape { | ||
| --page-margin: 15mm; | ||
| --page-gutter: 10mm; | ||
| } |
| .nospecidalmovement_imageOnTop .bloom-imageContainer { | ||
| height: calc(45% - var(--multilingual-editable-vertical-gap)); | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| top: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-translationGroup { | ||
| height: 50%; | ||
| width: 100%; | ||
| position: absolute; | ||
| left: 0; | ||
| bottom: 0; | ||
| } | ||
| .nospecidalmovement_imageOnTop .bloom-editable.bloom-monolingual { | ||
| height: 100%; | ||
| } | ||
| .bloom-bilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(50% - var(--multilingual-editable-vertical-gap)); | ||
| } | ||
| .bloom-trilingual.nospecidalmovement_imageOnTop .bloom-editable { | ||
| height: calc(33% - var(--multilingual-editable-vertical-gap)); | ||
| } |
| /* these rules allow different branding insertions depending on orientation */ | ||
| .portraitOnly, | ||
| .landscapeOnly { | ||
| display: none; | ||
| } | ||
| .Size6x9Portrait .portraitOnly, | ||
| .USComicPortrait .portraitOnly, | ||
| .Device16x9Portrait .portraitOnly, | ||
| .QuarterLetterPortrait .portraitOnly, | ||
| .HalfLetterPortrait .portraitOnly, | ||
| .LetterPortrait .portraitOnly, | ||
| .LegalPortrait .portraitOnly, | ||
| .A3Portrait .portraitOnly, | ||
| .A4Portrait .portraitOnly, | ||
| .A5Portrait .portraitOnly, | ||
| .B5Portrait .portraitOnly, | ||
| .A6Portrait .portraitOnly { | ||
| display: unset; | ||
| } | ||
| .Size6x9Landscape .landscapeOnly, | ||
| .Cm13Landscape .landscapeOnly, | ||
| .Device16x9Landscape .landscapeOnly, | ||
| .LetterLandscape .landscapeOnly, | ||
| .LegalLandscape .landscapeOnly, | ||
| .A3Landscape .landscapeOnly, | ||
| .A4Landscape .landscapeOnly, | ||
| .A5Landscape .landscapeOnly, | ||
| .A6Landscape .landscapeOnly { | ||
| display: unset; | ||
| } |
| .outsideBackCover img.branding { | ||
| margin-top: 4px; | ||
| width: 10cm; | ||
| } |
|
|
||
| <div class="bloom-editable bloom-nodefaultstylerule Title-On-Cover-style bloom-padForOverflow bloom-visibility-code-on bloom-content1 bloom-contentNational1 bloom-contentFirst" | ||
| lang="en" contenteditable="true" data-book="bookTitle" style="padding-bottom: 0px;" tabindex="0" | ||
| spellcheck="false" role="textbox" aria-label="false" data-hasqtip="true" aria-describedby="qtip-0"> |
| </div> | ||
|
|
||
| <div class="bloom-imageContainer"> | ||
| <img data-book="coverImage" src="placeHolder.png" alt=""></img> |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
src/stories/BloomPlayerTester.ts (2)
76-76: Address the TODO comment about locationOfDistFolder.The comment suggests using a proxy instead of the locationOfDistFolder parameter.
Would you like me to help create a proxy configuration in the Storybook setup to eliminate this parameter?
119-122: Replace magic number with named constant.The pause duration of 300ms is hardcoded without explanation.
+const DEFAULT_PAUSE_DURATION_MS = 300; async function pause() { - await new Promise((resolve) => setTimeout(resolve, 300)); + await new Promise((resolve) => setTimeout(resolve, DEFAULT_PAUSE_DURATION_MS)); }src/navigation.test.ts (1)
8-14: Consider using beforeAll for one-time setup.The history clearing in beforeEach might be more appropriate in beforeAll if the history state is not modified by individual tests.
+beforeAll(() => { + // Clear navigation history once before all tests + while (canGoBack()) { + tryPopPlayerHistory("dummy"); + } +}); -beforeEach(() => { - // Clear navigation history before each test - while (canGoBack()) { - tryPopPlayerHistory("dummy"); - } -});src/bloom-player-controls.tsx (1)
911-926: Improve readability of back button state logic.The nested if-else conditions make the logic harder to follow.
Consider extracting the logic into a more readable structure:
-getBackButtonState={() => { - if (coreRef.current?.CanGoBack()) - return BackButtonState.showArrow; - else if (showBackButton) { - if (window === window.top) - return BackButtonState.showArrow; - else return BackButtonState.showEllipsis; - } - else return BackButtonState.showNothing; -}} +getBackButtonState(): BackButtonState { + if (coreRef.current?.CanGoBack()) { + return BackButtonState.showArrow; + } + + if (!showBackButton) { + return BackButtonState.showNothing; + } + + return window === window.top + ? BackButtonState.showArrow + : BackButtonState.showEllipsis; +}src/bloom-player-core.tsx (3)
Line range hint
1760-1775: Consider future-proofing the RTL implementationThe current RTL implementation works but relies on swiper's internal implementation. Consider:
- Adding a TODO to track the upgrade to newer swiper version with built-in RTL support
- Documenting the current approach for maintainability
if (this.metaDataObject?.isRtl && this.swiperInstance) { + // TODO: Replace this implementation with Swiper's built-in RTL support when upgrading to newer version + // Current approach uses internal Swiper properties which may change in future versions this.swiperInstance.el?.setAttribute("dir", "rtl"); this.swiperInstance.rtl = true; this.swiperInstance.rtlTranslate = true; }
907-910: Consider using a more explicit map initializationThe current map initialization could be more explicit:
pageIdToIndex.length = 0is an unusual way to clear a map- Consider using a new object instead
-// clear the pageIdToIndex map -pageIdToIndex.length = 0; -pageIdToIndex["cover"] = 0; +// Initialize the pageIdToIndex map +const pageIdToIndex: { [key: string]: number } = { + cover: 0 +};
1030-1039: Enhance error message for page ID lookupConsider providing a more descriptive error message to help with debugging:
if (pageIdToIndex[this.state.startPageId] === undefined) { throw new Error( - `Page ID ${this.state.startPageId} not found in the current pageIdToIndexMap`, + `Page ID ${this.state.startPageId} not found in the current pageIdToIndexMap. ` + + `Available page IDs: ${Object.keys(pageIdToIndex).join(", ")}` ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/bloom-player-controls.tsx(5 hunks)src/bloom-player-core.tsx(25 hunks)src/externalContext.ts(1 hunks)src/navigation.test.ts(1 hunks)src/navigation.ts(1 hunks)src/stories/BloomPlayerTester.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/externalContext.ts
- src/navigation.ts
🔇 Additional comments (7)
src/stories/BloomPlayerTester.ts (1)
1-32: 🛠️ Refactor suggestion
Consider adding error handling for iframe access.
The initialization logic assumes the iframe's contentDocument will be accessible. However, cross-origin restrictions might prevent access to the iframe's content.
Add try-catch blocks around iframe access:
if (iframe) {
+ try {
if (iframe.contentDocument) {
this.iframeDoc = iframe.contentDocument;
return;
}
this.iframeDoc = await new Promise<Document>((resolve) => {
iframe.onload = () => resolve(iframe.contentDocument!);
});
return;
+ } catch (e) {
+ throw new Error(`Cannot access iframe content: ${e.message}`);
+ }
}src/bloom-player-controls.tsx (1)
1041-1041:
Breaking change: Rename startPage prop to startPageIndex.
The code still uses the old startPage prop name while passing it as startPageIndex to the core component.
This is a duplicate of the previous review comment. The breaking change needs to be addressed by updating the prop name consistently across the codebase.
src/bloom-player-core.tsx (5)
80-85: LGTM: Clean interface changes for navigation support
The new imports and interface changes are well-structured and properly support the navigation functionality:
- Modular approach with navigation-specific functions
- Clear state management for book URL and page tracking
Also applies to: 183-187, 226-228
355-369: LGTM: Clean implementation of navigation handlers
The navigation implementation is well-structured:
- Prevents unwanted swiper behavior for links
- Properly handles both book and page navigation
- Clear separation of concerns between event handling and navigation logic
Also applies to: 374-383
422-445: LGTM: Well-implemented back button support
The back button implementation is clean and follows good practices:
- Clear method names
- Proper separation between checking and handling
- Consistent error handling
555-561: LGTM: Improved URL handling
The URL handling improvements are well-implemented:
- Properly handles both relative and absolute paths
- Clear logic for handling leading slashes
2184-2191:
Handle potential null value when accessing page ID
The id attribute may be null. Consider adding error handling to avoid potential runtime errors.
Apply this diff:
-return bloomPage.getAttribute("id")!;
+const pageId = bloomPage.getAttribute("id");
+if (!pageId) {
+ throw new Error("No page ID found for index " + index);
+}
+return pageId;
src/stories/BloomPlayerTester.ts
Outdated
| async clickLinkByText(text: string) { | ||
| await pause(); | ||
| const link = await waitFor( | ||
| () => { | ||
| const links = Array.from(this.iframeDoc.querySelectorAll("a")); | ||
| const link = links.find((el) => el.textContent?.includes(text)); | ||
| if (!link) | ||
| throw new Error(`Link containing "${text}" not found`); | ||
| return link; | ||
| }, | ||
| { timeout: 5000 }, | ||
| ); | ||
| // the "pointerdown" event fails to trigger with the normal click here, | ||
| // which probably does a number of things, at least mousedown and up. | ||
| // As a workaround, we're sending the raw "click" event. | ||
| await fireEvent.click(link as HTMLElement); | ||
| //await userEvent.click(link as HTMLElement); | ||
| } | ||
|
|
||
| async goToNextPage() { | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| const rightButton = await waitFor(() => { | ||
| return this.iframeDoc.documentElement.querySelector( | ||
| ".swiper-button-right", | ||
| ); | ||
| }); | ||
| await userEvent.click(rightButton!); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve click event handling reliability.
The comment about pointerdown event failure suggests potential reliability issues with the click event handling.
Consider using a more robust click simulation:
-await fireEvent.click(link as HTMLElement);
+// Simulate full click sequence
+await fireEvent.pointerDown(link as HTMLElement);
+await fireEvent.pointerUp(link as HTMLElement);
+await fireEvent.click(link as HTMLElement);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async clickLinkByText(text: string) { | |
| await pause(); | |
| const link = await waitFor( | |
| () => { | |
| const links = Array.from(this.iframeDoc.querySelectorAll("a")); | |
| const link = links.find((el) => el.textContent?.includes(text)); | |
| if (!link) | |
| throw new Error(`Link containing "${text}" not found`); | |
| return link; | |
| }, | |
| { timeout: 5000 }, | |
| ); | |
| // the "pointerdown" event fails to trigger with the normal click here, | |
| // which probably does a number of things, at least mousedown and up. | |
| // As a workaround, we're sending the raw "click" event. | |
| await fireEvent.click(link as HTMLElement); | |
| //await userEvent.click(link as HTMLElement); | |
| } | |
| async goToNextPage() { | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| const rightButton = await waitFor(() => { | |
| return this.iframeDoc.documentElement.querySelector( | |
| ".swiper-button-right", | |
| ); | |
| }); | |
| await userEvent.click(rightButton!); | |
| } | |
| async clickLinkByText(text: string) { | |
| await pause(); | |
| const link = await waitFor( | |
| () => { | |
| const links = Array.from(this.iframeDoc.querySelectorAll("a")); | |
| const link = links.find((el) => el.textContent?.includes(text)); | |
| if (!link) | |
| throw new Error(`Link containing "${text}" not found`); | |
| return link; | |
| }, | |
| { timeout: 5000 }, | |
| ); | |
| // the "pointerdown" event fails to trigger with the normal click here, | |
| // which probably does a number of things, at least mousedown and up. | |
| // As a workaround, we're sending the raw "click" event. | |
| await fireEvent.pointerDown(link as HTMLElement); | |
| await fireEvent.pointerUp(link as HTMLElement); | |
| await fireEvent.click(link as HTMLElement); | |
| //await userEvent.click(link as HTMLElement); | |
| } | |
| async goToNextPage() { | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| const rightButton = await waitFor(() => { | |
| return this.iframeDoc.documentElement.querySelector( | |
| ".swiper-button-right", | |
| ); | |
| }); | |
| await userEvent.click(rightButton!); | |
| } |
| const windowSpy = vi | ||
| .spyOn(window, "open") | ||
| .mockImplementation(() => null); | ||
| const event = createClickEvent("https://example.com"); | ||
| const result = checkClickForBookOrPageJump( | ||
| event, | ||
| "doesn't matter", | ||
| () => "doesn't matter", | ||
| ); | ||
|
|
||
| expect(result).toEqual({}); | ||
| expect(windowSpy).toHaveBeenCalledWith( | ||
| "https://example.com", | ||
| "_blank", | ||
| "noreferrer", | ||
| ); | ||
| windowSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure mock cleanup in all test paths.
The window.open mock cleanup might not occur if the test fails.
+let windowSpy: any;
+
+beforeEach(() => {
+ windowSpy = vi.spyOn(window, 'open').mockImplementation(() => null);
+});
+
+afterEach(() => {
+ windowSpy.mockRestore();
+});
test("handles external links", () => {
- const windowSpy = vi
- .spyOn(window, "open")
- .mockImplementation(() => null);
const event = createClickEvent("https://example.com");
const result = checkClickForBookOrPageJump(
event,
"doesn't matter",
() => "doesn't matter",
);
expect(result).toEqual({});
expect(windowSpy).toHaveBeenCalledWith(
"https://example.com",
"_blank",
"noreferrer",
);
- windowSpy.mockRestore();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const windowSpy = vi | |
| .spyOn(window, "open") | |
| .mockImplementation(() => null); | |
| const event = createClickEvent("https://example.com"); | |
| const result = checkClickForBookOrPageJump( | |
| event, | |
| "doesn't matter", | |
| () => "doesn't matter", | |
| ); | |
| expect(result).toEqual({}); | |
| expect(windowSpy).toHaveBeenCalledWith( | |
| "https://example.com", | |
| "_blank", | |
| "noreferrer", | |
| ); | |
| windowSpy.mockRestore(); | |
| }); | |
| let windowSpy: any; | |
| beforeEach(() => { | |
| windowSpy = vi.spyOn(window, 'open').mockImplementation(() => null); | |
| }); | |
| afterEach(() => { | |
| windowSpy.mockRestore(); | |
| }); | |
| test("handles external links", () => { | |
| const event = createClickEvent("https://example.com"); | |
| const result = checkClickForBookOrPageJump( | |
| event, | |
| "doesn't matter", | |
| () => "doesn't matter", | |
| ); | |
| expect(result).toEqual({}); | |
| expect(windowSpy).toHaveBeenCalledWith( | |
| "https://example.com", | |
| "_blank", | |
| "noreferrer", | |
| ); | |
| }); |
src/bloom-player-controls.tsx
Outdated
| backClicked={() => { | ||
| if ( | ||
| !coreRef.current?.HandleBackButtonClickedIfHavePlayerHistory() | ||
| ) { | ||
| informHostOfBackAction(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for back button click failures.
The back button click handler doesn't handle potential errors from HandleBackButtonClickedIfHavePlayerHistory.
backClicked={() => {
+ try {
if (
!coreRef.current?.HandleBackButtonClickedIfHavePlayerHistory()
) {
informHostOfBackAction();
}
+ } catch (error) {
+ console.error('Failed to handle back button click:', error);
+ // Fallback to informing host
+ informHostOfBackAction();
+ }
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| backClicked={() => { | |
| if ( | |
| !coreRef.current?.HandleBackButtonClickedIfHavePlayerHistory() | |
| ) { | |
| informHostOfBackAction(); | |
| } | |
| }} | |
| backClicked={() => { | |
| try { | |
| if ( | |
| !coreRef.current?.HandleBackButtonClickedIfHavePlayerHistory() | |
| ) { | |
| informHostOfBackAction(); | |
| } | |
| } catch (error) { | |
| console.error('Failed to handle back button click:', error); | |
| // Fallback to informing host | |
| informHostOfBackAction(); | |
| } | |
| }} |
hatton
force-pushed
the
pageAndBookNavigation
branch
from
3193db6 to
514aa4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (24)
public/testBooks/multibook-index/basePage.css (3)
1-4: Enhance font stack with modern system fontsConsider expanding the font fallback chain to include modern system UI fonts for better cross-platform consistency.
body { line-height: 1.5; - font-family: Andika, "Andika New Basic", "Andika Basic", "Gentium Basic", "Gentium Book Basic", "Doulos SIL", Sans-Serif; + font-family: Andika, "Andika New Basic", "Andika Basic", "Gentium Basic", "Gentium Book Basic", "Doulos SIL", system-ui, -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", Arial, Sans-Serif; }
261-266: Use CSS variables for transform calculationsThe transform values contain magic numbers that would be more maintainable if defined as CSS variables with clear documentation.
+:root { + /* Scale factors for full bleed pages */ + --a5-scale: 1.04054054; + --a5-translate: 1.25675676mm; +} .bloom-fullBleed .bloom-page.A5Portrait { - transform: translate(0, -1.25675676mm) scale(1.04054054); + transform: translate(0, calc(-1 * var(--a5-translate))) scale(var(--a5-scale)); } .bloom-fullBleed .bloom-page.A5Landscape { - transform: translate(-1.25675676mm, 0) scale(1.04054054); + transform: translate(calc(-1 * var(--a5-translate)), 0) scale(var(--a5-scale)); }
1213-1216: Improve readability of complex calculationsThe margin and padding calculations are complex and could be more maintainable if broken down into intermediate variables with descriptive names.
.mixinBottomTgPadding { - --amountOfExtraMarginAddedForPageNumber: calc(var(--pageNumber-extra-height) * var(--pageNumber-show-multiplicand)); - --totalBottomMargin: calc(var(--page-margin-bottom) + var(--amountOfExtraMarginAddedForPageNumber)); - --amountOfVisualPaddingProvidedByBottomMargin: calc(var(--totalBottomMargin) * var(--page-and-marginBox-are-same-color-multiplicand)); - padding-bottom: calc(var(--topLevel-text-padding-bottom) - var(--amountOfVisualPaddingProvidedByBottomMargin)); + /* Extra margin for page numbers when they are shown */ + --page-number-margin: calc(var(--pageNumber-extra-height) * var(--pageNumber-show-multiplicand)); + + /* Total margin including page numbers */ + --total-bottom-margin: calc(var(--page-margin-bottom) + var(--page-number-margin)); + + /* Visual padding adjustment based on margin box color */ + --margin-visual-padding: calc(var(--total-bottom-margin) * var(--page-and-marginBox-are-same-color-multiplicand)); + + /* Final padding calculation */ + padding-bottom: calc(var(--topLevel-text-padding-bottom) - var(--margin-visual-padding)); }public/testBooks/multibook-index/Basic Book.css (2)
1-7: Consider using relative units for better responsivenessThe
.translatorsclass uses a fixed height of1.6cm. Consider using relative units (e.g.,emorrem) for better responsiveness across different screen sizes and zoom levels..translators { text-align: center; - height: 1.6cm; + height: 2.5rem; position: relative; }
154-157: Enhance documentation for vertical centering implementationThe comment about vertical centering could be more helpful by providing a specific reference to the JavaScript implementation.
-/*Text Whole Page -The current version of this is centered vertically; if you're looking for how that is done, it's not here, but rather -in javascript. Search for 'bloom-centerContentVertically'. -*/ +/* Text Whole Page + * Vertical centering is implemented in JavaScript using the 'bloom-centerContentVertically' class. + * @see src/bloom-player-core.tsx - handleCenterContentVertically() + */.storybook/main.ts (2)
23-26: Consider making book mappings more maintainable.The hardcoded book ID to name mapping could become difficult to maintain. Consider moving this to a separate configuration file.
-const idToBookName = { - "2e492eb1-bcc5-4b2b-b756-6cda33e1eee4": "multibook-index", - "2c1b71ac-f399-446d-8398-e61a8efd4e83": "multibook-target1", -}; +import { bookMappings } from '../config/storybook-book-mappings'; +const idToBookName = bookMappings;
46-49: Remove commented console.log statements.Either remove or uncomment the logging statements. Commented-out code should not be committed.
public/testBooks/multibook-index/meta.json (2)
1-1: Consider PII implications in location dataThe file contains specific location information (
country,province,district). Consider:
- Whether this level of detail is necessary
- Privacy implications for users
- GDPR compliance if deployed in EU
1-1: Standardize boolean field valuesSome boolean fields use explicit
falsewhile others arenull. Consider standardizing the approach:
a11y_NoEssentialInfoByColor: falsehazards: nulla11yFeatures: null-"hazards":null, -"a11yFeatures":null, +"hazards":false, +"a11yFeatures":false,README-advanced.md (1)
28-28: Improve sentence clarity and grammarConsider rephrasing the sentence for better readability. Replace "so as to" with "to", "a variety of" with "various", and use "and" after "both".
Apply this diff to enhance the sentence:
-Bloom-player is designed to be published using `npm` so as to be readily available to a variety of clients. The published version includes both `dist/bloomPlayer-HASH.js`, as well as some mp3 files used by activities. +Bloom-player is designed to be published using `npm` to be readily available to various clients. The published version includes both `dist/bloomPlayer-HASH.js` and some mp3 files used by activities.🧰 Tools
🪛 LanguageTool
[style] ~28-~28: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...is designed to be published usingnpmso as to be readily available to a variety of cl...(SO_AS_TO)
[style] ~28-~28: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...npmso as to be readily available to a variety of clients. The published version includes...(A_VARIETY_OF)
[grammar] ~28-~28: Probable usage error. Use “and” after ‘both’.
Context: ...cludes bothdist/bloomPlayer-HASH.js, as well as some mp3 files used by activities.W...
(BOTH_AS_WELL_AS)
src/navigation.ts (1)
59-69: Reuse 'goBackInHistoryIfPossible' to reduce code duplicationThe logic for handling 'back' navigation duplicates code from
goBackInHistoryIfPossible. Consider using the existing function to promote code reuse.Apply this diff to refactor the code:
if (href === "back") { - const previousLocation = jumpHistory.pop(); - if (previousLocation) { - return { - bookUrl: - currentBookInstanceId === previousLocation.bookId - ? undefined // we don't want to switch books, just pages - : urlFromBookId(previousLocation.bookId!), - pageId: previousLocation.pageId, - }; - } + const location = goBackInHistoryIfPossible(currentBookInstanceId); + if (location) { + return location; + } } else if (href.startsWith("/book/")) {src/navigation.test.ts (2)
9-14: Consider using a more robust cleanup approach.The current cleanup in
beforeEachrelies on the functions being tested, which could mask issues if these functions have bugs.Consider using a more direct approach to reset the history:
- beforeEach(() => { - // Clear navigation history before each test - while (canGoBack()) { - goBackInHistoryIfPossible("dummy"); - } - }); + beforeEach(() => { + vi.resetModules(); + // Reset the navigation history directly + vi.stubGlobal('navigationHistory', []); + });
138-152: Consider enhancing the mock event helper.The
createClickEventhelper could be more robust by including additional event properties that might be needed in future tests.Consider enhancing the helper:
function createClickEvent(href: string) { const mockElement = { attributes: { href: { nodeValue: href }, }, closest: () => mockElement, + getAttribute: (attr: string) => attr === 'href' ? href : null, + dataset: {}, }; return { target: mockElement, preventDefault: () => {}, stopPropagation: () => {}, + currentTarget: mockElement, + type: 'click', + bubbles: true, + cancelable: true, }; }README.md (2)
57-67: Add missing hyphen in 'upper left'.The term should be hyphenated when used as a compound adjective.
-If true, displays a button in the upper left corner of the app bar. +If true, displays a button in the upper-left corner of the app bar.Also, consider adding a comma after "However" in the note section for better readability.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~57-~57: Consider adding a hyphen.
Context: ...tton If true, displays a button in the upper left corner of the app bar. When clicked, bl...(TOP_LEFT_CORNER)
[uncategorized] ~64-~64: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...mally this button will be a left arrow. However if bloom-player is the top level window...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~64-~64: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t arrow. However if bloom-player is the top level window (i.e. it is not embedded in anot...(EN_COMPOUND_ADJECTIVE_INTERNAL)
151-154: Consider adding troubleshooting section.The testing instructions could benefit from common troubleshooting scenarios.
Consider adding:
4. If the preview doesn't load: - Ensure Bloom Editor is running and in Preview mode - Check the browser console for any CORS-related errors - Verify the preview URL matches the expected formatpublic/testBooks/multibook-target1/appearance.css (2)
103-103: Fix incomplete comment.The comment for
--marginBox-background-coloris incomplete and missing its closing quote.--- marginBox-background-color: white; /* it is not transparent because at edit time we set the parent (the translationGroup) color to show padding +-- marginBox-background-color: white; /* it is not transparent because at edit time we set the parent (the translationGroup) color to show padding */
115-117: Fix line break in comment.The comment has an unintended line break that splits the word "edit-time".
- /* In some themes, at edit- - time, the format button lands under the page number. See BL-13206. The name is is intentionally vague about direction and what we do with it. */ + /* In some themes, at edit-time, the format button lands under the page number. See BL-13206. The name is is intentionally vague about direction and what we do with it. */src/controlBar.tsx (2)
39-39: Remove unused import.The
Buttonimport from@material-ui/coreis not used in the code.-import { Button } from "@material-ui/core";
Line range hint
254-296: Consider extracting back button rendering logic.The back button rendering logic contains nested conditional expressions that could be simplified for better readability.
Consider extracting the icon rendering into a separate function:
const renderBackButtonIcon = (state: BackButtonState, preferredLanguages: string[]) => { switch (state) { case BackButtonState.showArrow: return ( <ArrowBack aria-label="Go Back" titleAccess={LocalizationManager.getTranslation( "Button.Back", preferredLanguages, "Back" )} /> ); case BackButtonState.showEllipsis: return ( <MoreHoriz aria-label="More Menu" titleAccess={LocalizationManager.getTranslation( "Button.More", preferredLanguages, "More" )} /> ); default: return null; } };Then use it in the JSX:
<IconButton color="secondary" onClick={() => props.backClicked?.()} data-testid="history-back-button" disabled={props.getBackButtonState() === BackButtonState.showNothing} > {renderBackButtonIcon(props.getBackButtonState(), props.preferredLanguages)} </IconButton>src/bloom-player-core.tsx (1)
1698-1832: Consider caching font stylesheet contentThe Andika font stylesheet is regenerated on each call to
makeAndikaStylesheet. Consider caching the stylesheet content as a constant since it's static.+const ANDIKA_FONT_STYLESHEET = ` + @font-face { + font-family: "Andika New Basic"; + // ... rest of the stylesheet content + } +`.replace("\n", "").replace("\r", ""); + private makeAndikaStylesheet(): void { let stylesheet = document.getElementById( "andikaCssStyleSheet", ) as HTMLStyleElement; if (!stylesheet) { stylesheet = document.createElement("style"); document.head.appendChild(stylesheet); stylesheet.setAttribute("id", "andikaCssStyleSheet"); } - stylesheet.innerText = ` - @font-face { - // ... current stylesheet content - } - `.replace("\n", "").replace("\r", ""); + stylesheet.innerText = ANDIKA_FONT_STYLESHEET; }public/testBooks/multibook-target1/origami.css (4)
46-48: Remove unnecessary commented-out code for cleaner maintenance.The commented-out code and notes at lines 46-47 are unnecessary and can be removed to improve code readability and maintainability.
Apply this diff to remove the comments:
cursor: ns-resize; z-index: 2; - /* JohnT mod: this extra padding makes it impossible to grab the crop control right above the split. - padding-bottom: 5px; /* this actually enlarges the target area for the user trying to find the splitter, without changing the layout of the page (because the position is absolute)*/ }
102-102: Avoid using!importantfor better CSS maintainability.Using
!importantcan make the stylesheet harder to maintain and override. Consider restructuring the CSS to eliminate the need for!important.
273-275: Clean up commented-out code to enhance readability.The commented lines at 273-275 seem to be leftover or unused code. Removing them will make the codebase cleaner.
Apply this diff:
- /*the commented line below would also add it to an empty bloom-content1 for re-entry to layout mode*/ - /*but that would be problematic/confusing in a multilingual context*/ - /*.origami-layout-mode .split-pane-component-inner .bloom-editable.bloom-content1:empty:before,*/
215-223: Ensure font sizes are responsive for better accessibility.The fixed font size of
12ptmay not provide optimal readability on all devices. Consider using relative units likeemorremfor scalability.Apply this diff to use relative units:
.split-pane-component-inner { position: relative; width: 100%; height: 100%; padding: 0px; box-sizing: border-box; transition-property: font-size; transition-duration: 0.5s; - font-size: 12pt; + font-size: 1em; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (16)
public/testBooks/multibook-index/BloomLocal.svgis excluded by!**/*.svgpublic/testBooks/multibook-index/back-cover-local.svgis excluded by!**/*.svgpublic/testBooks/multibook-index/happy-pirate.pngis excluded by!**/*.pngpublic/testBooks/multibook-index/history.dbis excluded by!**/*.dbpublic/testBooks/multibook-index/image.jpgis excluded by!**/*.jpgpublic/testBooks/multibook-index/license.pngis excluded by!**/*.pngpublic/testBooks/multibook-index/placeHolder.pngis excluded by!**/*.pngpublic/testBooks/multibook-index/thumbnail.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/BloomLocal.svgis excluded by!**/*.svgpublic/testBooks/multibook-target1/back-cover-local.svgis excluded by!**/*.svgpublic/testBooks/multibook-target1/history.dbis excluded by!**/*.dbpublic/testBooks/multibook-target1/image.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/license.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/placeHolder.pngis excluded by!**/*.pngpublic/testBooks/multibook-target1/thumbnail.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (45)
.storybook/main.ts(2 hunks).storybook/preview.ts(1 hunks)README-advanced.md(2 hunks)README.md(4 hunks)package.json(2 hunks)public/testBooks/multibook-index/Basic Book.css(1 hunks)public/testBooks/multibook-index/Traditional-XMatter.css(1 hunks)public/testBooks/multibook-index/appearance.css(1 hunks)public/testBooks/multibook-index/appearance.json(1 hunks)public/testBooks/multibook-index/basePage.css(1 hunks)public/testBooks/multibook-index/book.userPrefs(1 hunks)public/testBooks/multibook-index/branding.css(1 hunks)public/testBooks/multibook-index/customCollectionStyles.css(1 hunks)public/testBooks/multibook-index/defaultLangStyles.css(1 hunks)public/testBooks/multibook-index/index.htm(1 hunks)public/testBooks/multibook-index/meta.json(1 hunks)public/testBooks/multibook-index/origami.css(1 hunks)public/testBooks/multibook-index/previewMode.css(1 hunks)public/testBooks/multibook-index/publish-settings.json(1 hunks)public/testBooks/multibook-target1/Basic Book.css(1 hunks)public/testBooks/multibook-target1/Traditional-XMatter.css(1 hunks)public/testBooks/multibook-target1/appearance.css(1 hunks)public/testBooks/multibook-target1/appearance.json(1 hunks)public/testBooks/multibook-target1/basePage.css(1 hunks)public/testBooks/multibook-target1/book.userPrefs(1 hunks)public/testBooks/multibook-target1/branding.css(1 hunks)public/testBooks/multibook-target1/customCollectionStyles.css(1 hunks)public/testBooks/multibook-target1/defaultLangStyles.css(1 hunks)public/testBooks/multibook-target1/index.htm(1 hunks)public/testBooks/multibook-target1/meta.json(1 hunks)public/testBooks/multibook-target1/origami.css(1 hunks)public/testBooks/multibook-target1/previewMode.css(1 hunks)public/testBooks/multibook-target1/publish-settings.json(1 hunks)src/bloom-player-controls.tsx(4 hunks)src/bloom-player-core.tsx(25 hunks)src/bloomPlayerAnalytics.tsx(1 hunks)src/bookInfo.ts(1 hunks)src/controlBar.tsx(5 hunks)src/externalContext.ts(1 hunks)src/navigation.test.ts(1 hunks)src/navigation.ts(1 hunks)src/stories/BloomPlayerIframe.tsx(1 hunks)src/stories/BloomPlayerTester.ts(1 hunks)src/stories/multiBook.stories.tsx(1 hunks)src/stories/navigation.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/testBooks/multibook-index/publish-settings.json
🚧 Files skipped from review as they are similar to previous changes (28)
- public/testBooks/multibook-target1/customCollectionStyles.css
- public/testBooks/multibook-index/customCollectionStyles.css
- src/bookInfo.ts
- public/testBooks/multibook-index/defaultLangStyles.css
- public/testBooks/multibook-target1/appearance.json
- public/testBooks/multibook-index/Traditional-XMatter.css
- public/testBooks/multibook-target1/defaultLangStyles.css
- public/testBooks/multibook-target1/Traditional-XMatter.css
- public/testBooks/multibook-target1/book.userPrefs
- src/stories/navigation.stories.tsx
- public/testBooks/multibook-index/book.userPrefs
- public/testBooks/multibook-index/origami.css
- public/testBooks/multibook-index/appearance.json
- src/bloomPlayerAnalytics.tsx
- public/testBooks/multibook-target1/previewMode.css
- src/stories/BloomPlayerIframe.tsx
- public/testBooks/multibook-index/previewMode.css
- .storybook/preview.ts
- src/externalContext.ts
- public/testBooks/multibook-index/branding.css
- public/testBooks/multibook-target1/publish-settings.json
- public/testBooks/multibook-target1/index.htm
- public/testBooks/multibook-target1/meta.json
- public/testBooks/multibook-index/index.htm
- src/stories/BloomPlayerTester.ts
- package.json
- src/stories/multiBook.stories.tsx
- src/bloom-player-controls.tsx
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~57-~57: Consider adding a hyphen.
Context: ...tton If true, displays a button in the upper left corner of the app bar. When clicked, bl...
(TOP_LEFT_CORNER)
[uncategorized] ~64-~64: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...mally this button will be a left arrow. However if bloom-player is the top level window...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~64-~64: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t arrow. However if bloom-player is the top level window (i.e. it is not embedded in anot...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[formatting] ~106-~106: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...r. However, Bloom Player needs your help, because it does not have a way to know what the...
(COMMA_BEFORE_BECAUSE)
[uncategorized] ~158-~158: Possible missing comma found.
Context: ...y. ### Running unit tests To run unit tests use yarn test. This will run all `*.t...
(AI_HYDRA_LEO_MISSING_COMMA)
README-advanced.md
[style] ~28-~28: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...is designed to be published using npm so as to be readily available to a variety of cl...
(SO_AS_TO)
[style] ~28-~28: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ... npm so as to be readily available to a variety of clients. The published version includes...
(A_VARIETY_OF)
[grammar] ~28-~28: Probable usage error. Use “and” after ‘both’.
Context: ...cludes both dist/bloomPlayer-HASH.js, as well as some mp3 files used by activities.
W...
(BOTH_AS_WELL_AS)
🪛 Markdownlint (0.37.0)
README.md
109-109: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
🪛 Biome (1.9.4)
src/navigation.ts
[error] 44-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/bloom-player-core.tsx
[error] 1894-1894: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (23)
public/testBooks/multibook-target1/basePage.css (1)
1-1304: Skipping CSS review as requested
As per your comment "we don't want to review html or css in this pr", I'll skip reviewing this CSS file. Consider configuring path filters in your CodeRabbit configuration to automatically exclude HTML/CSS files from future reviews.
public/testBooks/multibook-index/basePage.css (1)
1-1304: Overall assessment: Well-structured and production-ready
The CSS codebase demonstrates good practices with:
- Effective use of modern CSS features (variables, calc, flexbox)
- Clear organization of styles by component and functionality
- Detailed comments explaining complex calculations
- Consistent naming conventions
The suggested improvements are primarily for maintainability and do not impact functionality.
public/testBooks/multibook-index/Basic Book.css (1)
65-87: Fix typo in class name 'nospecidalmovement'
The class name contains a typo: 'nospecidalmovement' should be 'nospecialmovement'.
.storybook/main.ts (3)
61-65:
Fix potential override of addons configuration.
Setting addons: ["@storybook/addon-interactions"] overwrites the existing addons array defined earlier.
return {
...config,
server: { ...config.server, proxy: updatedProxy },
- addons: ["@storybook/addon-interactions"],
+ addons: [...(config.addons || []), "@storybook/addon-interactions"],
};38-50:
Add error handling for unknown book IDs.
The rewrite function should handle cases where the book ID is not found in the mapping.
rewrite: (path) => {
const bookId = path.split("/")[2];
const bookName = idToBookName[bookId];
+ if (!bookName) {
+ console.error(`Unknown book ID: ${bookId}`);
+ return path;
+ }
const rewrittenPath = path.replace(
`/book/${bookId}`,
`testBooks/${bookName}`,
);
return rewrittenPath;
},Likely invalid or redundant comment.
10-12: LGTM! Note the build requirement.
The static directory configuration is well-documented. Make sure to run yarn buildForStorybook when working with iframe stories.
Let's verify if the build script exists:
✅ Verification successful
Configuration is correct and necessary for iframe-based stories
The setup is verified and appropriate:
- The
buildForStorybookscript exists and runsvite build --watch --mode development - The
distdirectory is properly gitignored - Multiple stories and components use iframes (notably
src/stories/BloomPlayerIframe.tsxand several story files) - The static directory configuration is necessary for these iframe-based stories to function correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the buildForStorybook script exists in package.json
jq '.scripts | has("buildForStorybook")' package.json
Length of output: 60
Script:
#!/bin/bash
# Let's check what the buildForStorybook script actually does
jq -r '.scripts.buildForStorybook' package.json
# Also check if the dist directory is mentioned in .gitignore
grep -l "dist" .gitignore
# Check if there are any stories using iframes
rg -l "iframe" "stories/"
Length of output: 200
Script:
#!/bin/bash
# Let's check where the stories are located (they might be in src/)
fd -t f "stories.tsx" -t f "stories.ts"
# Also check if there are any iframe references in any tsx/ts files
rg -l "iframe" -g "*.{tsx,ts}"
Length of output: 576
public/testBooks/multibook-index/meta.json (2)
1-1: Review security implications of exposed IDs
The bookInstanceId and bookLineage contain UUIDs that might be sensitive. Consider:
- Whether these IDs should be exposed in public metadata
- If they're used in any security-critical operations
1-1: Verify required metadata fields
Several critical metadata fields are empty or null:
languagesarray is empty butlanguage-display-namescontains Englishcopyright,author, andpublisherare nulltagsandfeaturesarrays are empty
These fields might be required for proper book indexing and discovery.
src/navigation.ts (1)
44-47: Simplify attribute access using optional chaining
You can simplify the attribute access by using optional chaining, improving readability.
Apply this diff to refactor the code:
const href: string =
- (linkElement.attributes["href"] &&
- linkElement.attributes["href"].nodeValue) ||
- (linkElement.attributes["data-href"] &&
- linkElement.attributes["data-href"].nodeValue);
+ linkElement.attributes["href"]?.nodeValue ||
+ linkElement.attributes["data-href"]?.nodeValue;🧰 Tools
🪛 Biome (1.9.4)
[error] 44-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/navigation.test.ts (2)
87-105: Mock cleanup might not occur if test fails.
The window.open mock cleanup might not occur if the test fails before reaching the cleanup line.
107-134: LGTM! Comprehensive test coverage for multiple back navigation steps.
The test thoroughly validates the navigation stack behavior with multiple steps and different scenarios.
README.md (1)
102-130: LGTM! Clear and comprehensive multi-book support documentation.
The section effectively explains:
- Purpose and context of book linking
- Instance ID concept and usage
- Step-by-step implementation guide
- Performance considerations
🧰 Tools
🪛 LanguageTool
[formatting] ~106-~106: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...r. However, Bloom Player needs your help, because it does not have a way to know what the...
(COMMA_BEFORE_BECAUSE)
🪛 Markdownlint (0.37.0)
109-109: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
public/testBooks/multibook-target1/Basic Book.css (1)
1-348: Skipping CSS review as requested.
As per previous feedback, HTML and CSS reviews are out of scope for this PR.
public/testBooks/multibook-target1/appearance.css (2)
136-137:
Remove duplicate selector.
The .LegalLandscape selector appears twice in the list.
.LetterLandscape,
.LetterPortrait,
-.LegalLandscape,
.LegalLandscape {Likely invalid or redundant comment.
201-201: Verify page number hiding approach.
The comment "review will this work to hide it?" suggests uncertainty about the approach to hide page numbers using font-size: 0px. Consider using display: none or visibility: hidden for more explicit hiding.
✅ Verification successful
Let me search for more context about how these CSS variables are used.
Using font-size: 0px is a valid approach for hiding page numbers.
Looking at the codebase, the page number visibility is controlled through multiple coordinated CSS variables:
--pageNumber-font-size: 0pxeffectively hides the text content--pageNumber-show-multiplicand: 0removes extra spacing--pageNumber-extra-height: 0mmeliminates additional height
This approach is already being used consistently in both multibook-target1 and multibook-index themes. While display: none could also work, the current solution is valid as it:
- Maintains the element's positioning in the layout
- Properly handles spacing through the multiplicand variables
- Integrates well with the existing CSS variable system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any other page number hiding approaches in the codebase.
# Search for page number hiding patterns
rg -A 2 'pageNumber.*hide|hide.*pageNumber'
Length of output: 756
Script:
#!/bin/bash
# Search for usage of pageNumber CSS variables and related styles
rg -A 3 'pageNumber|\.pagenumber' --type css
Length of output: 26357
src/controlBar.tsx (1)
50-54: LGTM: Well-defined enum for back button states.
The BackButtonState enum clearly defines the possible states for the back button, making the code more maintainable and type-safe.
src/bloom-player-core.tsx (4)
2489-2495: Handle potential null value when accessing page ID
The id attribute may be null. Consider adding error handling to avoid potential runtime errors.
409-434: LGTM: Clean navigation implementation
The navigation implementation is well-structured:
- Clear separation of concerns with external navigation module
- Proper state management for book URL and page tracking
- Consistent error handling
1655-1696: LGTM: Robust font handling implementation
The font handling implementation is well-designed:
- Proper URL transformation for font files
- Good error handling for missing fonts.css
- Clear documentation of security considerations
2364-2394: Verify if fixInternalHyperlinks functionality is still needed
The fixInternalHyperlinks method is commented out. If this functionality is still needed, consider:
- Re-implementing it using the new navigation system
- Removing the commented code if it's obsolete
✅ Verification successful
Let me verify if the fixInternalHyperlinks functionality is still needed by checking if there are any internal hyperlinks in the codebase and if there is any alternative implementation.
Let me try different search patterns and file types to check for internal hyperlink handling:
Let me try one more search pattern to look for internal navigation handling:
Based on the search results, I can now provide a definitive response about the commented-out fixInternalHyperlinks method:
Internal hyperlink handling has been moved to the navigation system
The commented-out fixInternalHyperlinks method is obsolete as internal navigation is now handled by a dedicated navigation system in src/navigation.ts. The new system:
- Properly handles internal page links through the
checkClickForBookOrPageJumpfunction - Maintains navigation state and history through
jumpHistory - Uses
pageIdToIndexMapto convert page IDs to indices for navigation - Supports both in-book (
#pageId) and cross-book (/book/bookId#pageId) navigation
The commented code can be safely removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any internal hyperlinks in the book that need handling
# Look for anchor tags with href starting with #
rg -A 2 "href=\"#"
# Look for any code that might be handling internal navigation
ast-grep --pattern 'href[^>]*#[^>]*'
Length of output: 131070
Script:
#!/bin/bash
# Look for internal hyperlink handling in the codebase
rg -A 2 "href=\"#" --type ts --type tsx --type js
# Look for any code that might be handling internal navigation
ast-grep --pattern 'slideTo($_, $_)'
# Look for any new implementation of internal hyperlink handling
rg -A 2 "href.*#" --type ts --type tsx --type js
Length of output: 196
Script:
#!/bin/bash
# Look for internal hyperlink handling in the codebase
rg -A 2 "href.*#"
# Look for any code that might be handling internal navigation
rg -A 2 "slideTo|navigateTo|internalLink"
# Look for any new implementation of internal hyperlink handling
rg -A 2 "handleLink|linkClick|anchorClick"
Length of output: 92477
Script:
#!/bin/bash
# Look for any code that handles page/slide navigation
rg -A 5 "pageIdToIndexMap|pageId|pageNum|pageIndex" src/
# Look for any code that handles anchor/link clicks
rg -A 5 "onClick|handleClick|clickHandler" src/
# Look for any code that handles navigation state
rg -A 5 "navigationState|currentPage|currentSlide" src/
Length of output: 60292
public/testBooks/multibook-target1/branding.css (3)
55-58: Consider using relative units instead of absolute units for responsiveness.
Using absolute units like cm may not adapt well to different screen sizes and resolutions. Switching to relative units like % can improve the responsiveness of the branding images across various devices.
Apply this diff:
.outsideBackCover img.branding {
margin-top: 4px;
- width: 10cm;
+ width: 100%;
}10-13: Verify the specificity of the attribute selector to prevent unintended styling.
Using [data-book*="branding"] may match more elements than intended due to the wildcard. Ensure that this selector targets only the desired elements to avoid unintended styles being applied elsewhere.
Run the following script to check for all elements affected by this selector:
✅ Verification successful
The attribute selector [data-book*="branding"] is consistently used across test books with the same styling pattern
The search results show that this selector is used consistently across multiple test books in the repository, always applying the same flex container styles and image alignment rules. The pattern appears in:
branding.cssfiles: Setting flex layout and image stylesbasePage.cssfiles: Resetting link styles within branding elements
The usage is uniform and intentional across test books, indicating this is a standardized styling approach rather than an overly broad selector. The wildcard selector is appropriate here as it's part of a consistent styling system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all elements with a 'data-book' attribute containing 'branding'
# Test: Search for affected elements in the codebase.
rg '\[data-book\*="branding"\]' -A 5
Length of output: 10475
28-54: 🛠️ Refactor suggestion
Simplify media queries for orientation-specific classes.
The classes for portrait and landscape orientations are listing multiple size-specific classes. Consider using media queries to handle orientation changes more efficiently.
Apply this diff to use media queries:
-.portraitOnly,
-.landscapeOnly {
- display: none;
-}
-
-.Size6x9Portrait .portraitOnly,
-...
-.A6Portrait .portraitOnly {
- display: unset;
-}
-
-.Size6x9Landscape .landscapeOnly,
-...
-.A6Landscape .landscapeOnly {
- display: unset;
-}
+
+@media (orientation: portrait) {
+ .portraitOnly {
+ display: block;
+ }
+}
+
+@media (orientation: landscape) {
+ .landscapeOnly {
+ display: block;
+ }
+}Likely invalid or redundant comment.
| border: none; | ||
| z-index: 1002; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement a z-index management system
The z-index values are hardcoded across different components without a clear system. Consider using CSS variables to manage z-index values consistently.
:root {
+ /* Z-index stack (from highest to lowest) */
+ --z-comical: 1002;
+ --z-video: 1002;
+ --z-widget: 1000;
+ --z-translation: 1002;
}
.bloom-imageContainer .bloom-textOverPicture .bloom-translationGroup {
border: none;
- z-index: 1002;
+ z-index: var(--z-translation);
}
.customPage .bloom-videoContainer {
box-sizing: border-box;
width: 100%;
height: 100%;
- z-index: 1002;
+ z-index: var(--z-video);
}
.customPage .bloom-widgetContainer {
box-sizing: border-box;
width: 100%;
height: 100%;
- z-index: 1000;
+ z-index: var(--z-widget);
}
.comical-generated {
- z-index: 1001;
+ z-index: var(--z-comical);
position: absolute;
top: 0;
left: 0;
}Also applies to: 868-869, 884-885, 966-967
| } | ||
| .pictureAndWordPage.QuarterLetterPortrait .bloom-translationGroup { | ||
| height: 35%; | ||
| } | ||
| .pictureAndWordPage.QuarterLetterPortrait .bloom-imageContainer { | ||
| height: 62%; | ||
| } | ||
| .pictureAndWordPage.QuarterLetterPortrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.QuarterLetterLandscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.QuarterLetterLandscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.QuarterLetterLandscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.HalfLetterPortrait .bloom-translationGroup { | ||
| height: 35%; | ||
| } | ||
| .pictureAndWordPage.HalfLetterPortrait .bloom-imageContainer { | ||
| height: 62%; | ||
| } | ||
| .pictureAndWordPage.HalfLetterPortrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.LetterPortrait .bloom-translationGroup { | ||
| height: 23%; | ||
| } | ||
| .pictureAndWordPage.LetterPortrait .bloom-imageContainer { | ||
| height: 74%; | ||
| } | ||
| .pictureAndWordPage.LetterPortrait .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.LetterLandscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.LetterLandscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.LetterLandscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.LegalPortrait .bloom-translationGroup { | ||
| height: 23%; | ||
| } | ||
| .pictureAndWordPage.LegalPortrait .bloom-imageContainer { | ||
| height: 74%; | ||
| } | ||
| .pictureAndWordPage.LegalPortrait .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.LegalLandscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.LegalLandscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.LegalLandscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } | ||
| .pictureAndWordPage.Cm13Landscape .bloom-translationGroup { | ||
| height: 37%; | ||
| } | ||
| .pictureAndWordPage.Cm13Landscape .bloom-imageContainer { | ||
| height: 60%; | ||
| } | ||
| .pictureAndWordPage.Cm13Landscape .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.USComicPortrait .bloom-translationGroup { | ||
| height: 32%; | ||
| } | ||
| .pictureAndWordPage.USComicPortrait .bloom-imageContainer { | ||
| height: 65%; | ||
| } | ||
| .pictureAndWordPage.USComicPortrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Portrait .bloom-translationGroup { | ||
| height: 35%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Portrait .bloom-imageContainer { | ||
| height: 62%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Portrait .bloom-content1 { | ||
| font-size: 400%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Landscape .bloom-translationGroup { | ||
| height: 42%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Landscape .bloom-imageContainer { | ||
| height: 55%; | ||
| } | ||
| .pictureAndWordPage.Size6x9Landscape .bloom-content1 { | ||
| font-size: 600%; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in page format styles
There's significant repetition in the styles for different page formats. Consider using CSS custom properties and creating a more maintainable structure.
+ :root {
+ --portrait-translation-height: 35%;
+ --portrait-image-height: 62%;
+ --landscape-translation-height: 42%;
+ --landscape-image-height: 55%;
+ --large-font-size: 600%;
+ --medium-font-size: 400%;
+ }
- .pictureAndWordPage.A6Portrait .bloom-translationGroup {
- height: 35%;
- }
- .pictureAndWordPage.A6Portrait .bloom-imageContainer {
- height: 62%;
- }
+ .pictureAndWordPage {
+ &[class*="Portrait"] {
+ .bloom-translationGroup {
+ height: var(--portrait-translation-height);
+ }
+ .bloom-imageContainer {
+ height: var(--portrait-image-height);
+ }
+ }
+ &[class*="Landscape"] {
+ .bloom-translationGroup {
+ height: var(--landscape-translation-height);
+ }
+ .bloom-imageContainer {
+ height: var(--landscape-image-height);
+ }
+ }
+ }This refactoring would significantly reduce code duplication and make the styles more maintainable. Consider using a CSS preprocessor like SASS/SCSS to better organize these patterns.
Committable suggestion skipped: line range outside the PR's diff.
| top: calc(13px + 24%); | ||
| height: 24%; | ||
| } | ||
| .bloom-trilingual.imageOnTop .bloom-editable { | ||
| height: 24%; | ||
| } | ||
| .bloom-trilingual.imageOnTop .bloom-content2 { | ||
| position: absolute; | ||
| top: calc(1.3% + 24% + 24%); | ||
| } | ||
| .bloom-trilingual.imageOnTop .bloom-content3 { | ||
| position: absolute; | ||
| bottom: 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace magic numbers with CSS variables
The trilingual layout calculations use hard-coded values that could be difficult to maintain. Consider using CSS variables for these values to improve maintainability and consistency.
:root {
+ --trilingual-spacing: 13px;
+ --trilingual-section-height: 24%;
}
.bloom-trilingual.imageOnTop .bloom-imageContainer {
- top: calc(13px + 24%);
+ top: calc(var(--trilingual-spacing) + var(--trilingual-section-height));
- height: 24%;
+ height: var(--trilingual-section-height);
}
.bloom-trilingual.imageOnTop .bloom-content2 {
position: absolute;
- top: calc(1.3% + 24% + 24%);
+ top: calc(var(--trilingual-spacing) + var(--trilingual-section-height) * 2);
}Committable suggestion skipped: line range outside the PR's diff.
| // setup the same proxy that we have in "vite dev" to avoid CORS issues. (THIS IS NOT WORKING) | ||
| // Note that it requires that paths to book on BloomLibrary.org start with "s3/" in place of "https://s3.amazonaws.com/" | ||
| ...proxy, |
There was a problem hiding this comment.
💡 Codebase verification
I have enough context now to provide a final response about the CORS setup issue.
The proxy configuration needs to be fixed for S3 access
The current proxy setup in .storybook/main.ts spreads the proxy configuration from vite.config.ts but doesn't properly handle the S3 proxy rule. Looking at the code:
- The S3 proxy rule from vite.config.ts is missing, which should handle
/s3paths - The comment indicates CORS issues with S3 resources aren't being resolved
To fix this, add the S3 proxy rule explicitly:
const updatedProxy = {
"/s3": {
target: "https://s3.amazonaws.com",
changeOrigin: true,
rewrite: (path) => path.replace(/^\/s3/, ""),
},
...proxy,
// rest of the proxy config
};🔗 Analysis chain
Address the non-working CORS setup.
The comment indicates that the CORS setup is not working. This should be investigated and fixed rather than leaving it as a known issue.
Let's check for CORS-related configurations:
Let me gather more context about the proxy configuration to better understand the CORS setup.
Let me check the vite configuration function to understand how the proxy is defined.
Let me check the vite configuration in the storybook's main.ts file to understand how the proxy is being imported and used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CORS-related configurations in the codebase
rg -i "cors" --type ts
Length of output: 136
Script:
#!/bin/bash
# Check the proxy configuration in both vite.config.ts and .storybook/main.ts
echo "=== vite.config.ts ==="
rg -A 10 "proxy.*=" vite.config.ts
echo -e "\n=== .storybook/main.ts ==="
rg -A 10 "proxy.*=" .storybook/main.ts
Length of output: 665
Script:
#!/bin/bash
# Look for viteConfigFn definition and its proxy configuration
echo "=== Searching for viteConfigFn ==="
rg -l "viteConfigFn"
echo -e "\n=== Content of files with viteConfigFn ==="
fd -t f -e ts -e js -x sh -c 'echo "=== {}" && cat {}'
Length of output: 68751
Script:
#!/bin/bash
# Look for the content of .storybook/main.ts
echo "=== Content of .storybook/main.ts ==="
cat .storybook/main.ts
Length of output: 2802
| @@ -0,0 +1 @@ | |||
| {"a11y_NoEssentialInfoByColor":false,"a11y_NoTextIncludedInAnyImages":false,"bookInstanceId":"2e492eb1-bcc5-4b2b-b756-6cda33e1eee4","suitableForMakingShells":false,"suitableForMakingTemplates":false,"suitableForVernacularLibrary":true,"bloomdVersion":0,"experimental":false,"brandingProjectName":"Local-Community","nameLocked":false,"folio":false,"isRtl":false,"title":"multibook-index","allTitles":"{\"en\":\"multibook-index\"}","originalTitle":"multibook-index","BaseUrl":null,"isbn":"","bookLineage":"056B6F11-4A6C-4942-B2BC-8861E62B03B3","license":null,"formatVersion":"2.1","licenseNotes":null,"copyright":null,"credits":"","tags":[],"pageCount":0,"languages":[],"languageDescriptors":null,"summary":null,"allowUploadingToBloomLibrary":true,"draft":false,"bookletMakingIsAppropriate":true,"country":"United States","province":"","district":"","tools":[],"currentTool":"talkingBookTool","toolboxIsOpen":false,"author":null,"publisher":null,"originalPublisher":null,"subjects":null,"hazards":null,"a11yFeatures":null,"a11yLevel":null,"a11yCertifier":null,"readingLevelDescription":null,"typicalAgeRange":null,"features":[],"page-number-style":"Decimal","language-display-names":{"en":"English"},"internetLimits":null,"use-original-copyright":false,"imported-book-source-url":null,"phashOfFirstContentImage":null} | |||
There was a problem hiding this comment.
💡 Codebase verification
Page count mismatch confirmed in meta.json
The book actually contains 8 pages as shown in index.htm, but meta.json incorrectly shows pageCount: 0. This should be updated to reflect the actual page count.
public/testBooks/multibook-index/meta.json: UpdatepageCountfrom 0 to 8 to match the actual number of pages found in index.htm.
🔗 Analysis chain
Verify page count accuracy
The pageCount is set to 0, which seems incorrect for a functional book. This might affect navigation functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for page counting logic
ast-grep --pattern 'pageCount = $_'
Length of output: 37
Script:
#!/bin/bash
# Look for files in the same directory to understand book structure
ls -la public/testBooks/multibook-index/
# Check if there are any page files in the directory
find public/testBooks/multibook-index/ -type f -name "*.htm" -o -name "*.html"
# Search for any page-related files or content
find public/testBooks/multibook-index/ -type f
Length of output: 2896
Script:
#!/bin/bash
# Let's examine the index.htm file to count actual pages
cat public/testBooks/multibook-index/index.htm | grep -c 'class="bloom-page'
# Also check if this is a test/sample book
cat public/testBooks/multibook-index/index.htm | head -n 20
Length of output: 674
| export function checkClickForBookOrPageJump( | ||
| event: any, | ||
| currentBookInstanceId: string, | ||
| getCurentPageId: () => string, |
There was a problem hiding this comment.
Correct typo in parameter name 'getCurentPageId'
There's a typo in the parameter name getCurentPageId. It should be getCurrentPageId for consistency.
Apply this diff to fix the typo:
export function checkClickForBookOrPageJump(
event: any,
currentBookInstanceId: string,
- getCurentPageId: () => string,
+ getCurrentPageId: () => string,
): ILocation {And update its usage inside the function:
- const currentPageId = getCurentPageId();
+ const currentPageId = getCurrentPageId();Committable suggestion skipped: line range outside the PR's diff.
| .A4Landscape, | ||
| .A4Portrait, | ||
| .B5Portrait, | ||
| .LetterLandscape, | ||
| .LetterPortrait, | ||
| .LegalLandscape, | ||
| .LegalLandscape { | ||
| --page-margin: 15mm; | ||
| --page-gutter: 10mm; | ||
| } | ||
| .HalfLetterPortrait,/* SIL Mexico says use same as A5 */ | ||
| .HalfLetterLandscape, | ||
| .A5Landscape, | ||
| .A5Portrait, | ||
| .Size6x9Landscape, | ||
| .Size6x9Portrait, | ||
| .USComicPortrait { | ||
| --page-margin: 12mm; | ||
| } | ||
| .QuarterLetterPortrait, | ||
| .QuarterLetterLandscape, | ||
| .A6Portrait, | ||
| .A6Landscape { | ||
| --page-margin: 10mm; | ||
| } | ||
| .Cm13Landscape { | ||
| --page-margin: 5mm; | ||
| } | ||
| .bloom-page[class*="Device"] { | ||
| --page-margin: 10px; | ||
| } | ||
|
|
||
| .bloom-page[class*="Device"], | ||
| .bloomPlayer-page .bloom-page { | ||
| /* Deliberately set both of these, which combines with a special device/bloom-player rule | ||
| * in basePage.less which sets both left and right margin to auto, and we get | ||
| * centered as the default for device/bloom-player pages. (To override this, you need to | ||
| * set the margin you don't want to something like "deliberately-invalid".)*/ | ||
| --pageNumber-right-margin: 0px; | ||
| --pageNumber-always-left-margin: 0px; | ||
| } | ||
|
|
||
| .numberedPage[class*="Device"] | ||
| { | ||
| /* These page sizes have small margins, so we need extra height on numbered ones. | ||
| (Note that this disappears if page numbers are turned off, because --pageNumber-show-multiplicand is 0.) */ | ||
| --pageNumber-extra-height: 8mm !important; | ||
| } | ||
|
|
||
|
|
||
| /* We need a small margin (above) for xmatter pages (e.g., Robots Mali credits page, | ||
| * especially in landscape, comment in BL-13002). | ||
| * But we need more on numbered pages, so we can fit the page number. | ||
| * This unfortunately breaks our rule about keeping the specificity at one class so that | ||
| * the settings dialog can override. But currently we don't expect to make page margins | ||
| * a book setting, so for now we're OK.*/ | ||
| /*.numberedPage.Device16x9Portrait, | ||
| .numberedPage.Device16x9Landscape { | ||
| --page-margin-bottom: 45px; /* need room for page number, so not as small as others | ||
| }*/ | ||
|
|
||
| /* classes are doubled in this rule because it needs to beat the one above that sets the | ||
| * bottom margin for numbered pages in Device page sizes. This makes it equally specific | ||
| * and thus the last one wins. */ | ||
| .no-margin-page.no-margin-page, | ||
| .comic.comic { | ||
| --page-margin-top: 0px; | ||
| --page-margin-bottom: 0px; | ||
| --page-margin-right: 0px; | ||
| --page-margin-left: 0px; | ||
| --pageNumber-font-size: 0px; /* review will this work to hide it? */ | ||
| --pageNumber-show-multiplicand: 0; /* no extra space for suppressed page number */ | ||
| --pageNumber-extra-height: 0mm !important; | ||
| } | ||
|
|
||
| /***************************************************************************** | ||
| * These variables / rules are for xmatter pages. | ||
| */ | ||
| .bloom-frontMatter, .bloom-backMatter { | ||
| --MarginBetweenXMatterBlocks: 2em; | ||
| --MarginBetweenXMatterMinorItems: 5px; | ||
| --DefaultCoverTitle1FontSize: 25pt; | ||
| --DefaultCoverTitle2FontSize: 12pt; | ||
| /* Same as our ordinary default, but we have to set it explicitly because of a trick | ||
| that copies the DefaultCoverTitle1FontSize to the translationGroup. */ | ||
| --DefaultCoverTitle3FontSize: 10pt; | ||
| } | ||
|
|
||
| /* From this book's appearance settings */ | ||
| .bloom-page { | ||
| --cover-title-L1-show: doShow-css-will-ignore-this-and-use-default; | ||
| --cover-title-L2-show: doShow-css-will-ignore-this-and-use-default; | ||
| --cover-title-L3-show: none; | ||
| --cover-topic-show: doShow-css-will-ignore-this-and-use-default; | ||
| --cover-languageName-show: doShow-css-will-ignore-this-and-use-default; | ||
| --pageNumber-show: doShow-css-will-ignore-this-and-use-default; | ||
| --cover-background-color: #d34b5f; | ||
| --pageNumber-show-multiplicand: 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing CSS duplication.
This file is identical to multibook-target1/appearance.css except for the cover background color (#d34b5f vs #7bdcb5). Consider extracting the common styles into a shared base CSS file and only override the book-specific values.
Suggested approach:
- Create a base
appearance.csswith all common styles - Create book-specific CSS files that only contain overrides:
/* book-specific.css */
.bloom-page {
--cover-background-color: #d34b5f; /* or #7bdcb5 for the other book */
}
This change is
Summary by CodeRabbit
New Features
BloomPlayerIframecomponent anduseBuildIsReadyhook for iframe integration.Bug Fixes
Documentation
Tests
Style