Convert Collection Selector from Modal to Route#1006
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR migrates collections/layout selection from a modal ( ChangesCollections layout selection: modal to route migration
Sequence DiagramsequenceDiagram
participant User
participant EntryPoint as Sidebar / TextPage / ContentsPage
participant Router as SvelteKit Router
participant CollectionsPage as /collections Page
participant LayoutOptions
participant CollectionModal
participant Stores as $refs / $layout
User->>EntryPoint: tap Layout / Collection Selector
EntryPoint->>Router: goto(resolve('/collections'))
Router->>CollectionsPage: render page
CollectionsPage->>Stores: read $refs.docSet, $selectedLayouts
CollectionsPage->>User: show Navbar + TabsMenu (Single/Two/VerseByVerse)
User->>CollectionsPage: select layout tab
CollectionsPage->>LayoutOptions: render active layout options
User->>LayoutOptions: click collection item (Two Pane / Verse By Verse)
LayoutOptions->>CollectionModal: modal.open(type, showBlank, index)
CollectionModal->>User: show modal with CollectionList
User->>CollectionModal: select doc set, click item
CollectionModal->>Stores: update $selectedLayouts[viewType]
CollectionModal->>User: close modal
User->>CollectionsPage: tap confirm (handleOk)
CollectionsPage->>Stores: write selected layout + doc sets to $refs/$layout
CollectionsPage->>Router: goto(/)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
src/lib/components/CollectionModal.svelteOops! Something went wrong! :( ESLint: 9.39.4 The requested operation requires ESLint to serialize configuration data, Please double-check your configuration for errors. If you still have problems, please stop by https://eslint.org/chat/help to chat src/lib/components/LayoutOptions.svelteOops! Something went wrong! :( ESLint: 9.39.4 The requested operation requires ESLint to serialize configuration data, Please double-check your configuration for errors. If you still have problems, please stop by https://eslint.org/chat/help to chat Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TheNonPirate
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/components/CollectionList.svelte (1)
40-47: ⚡ Quick winConsider using
<button>or adding keyboard event handlers for better accessibility.The switch from
<a>to<div role="button">with suppresseda11y_click_events_have_key_eventsanda11y_interactive_supports_focuswarnings degrades keyboard navigation. Users relying on keyboard-only interaction may not be able to activate these collection items.♿ Recommended fix: use a native button element
- <div + <button onclick={() => handleClick(d)} style:background-color={selectedLayouts?.id === d.id ? $themeColors['LayoutItemSelectedBackgroundColor'] : ''} class="flex justify-between layout-item-block rounded-none" - role="button" >If you must keep the
<div>, add anonkeydownhandler to support Enter/Space key activation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/CollectionList.svelte` around lines 40 - 47, Replace the `<div role="button">` element with a native `<button>` element to improve keyboard accessibility. Remove the `role="button"` attribute since native buttons have built-in keyboard support for Enter and Space keys. Keep the existing `onclick` handler (or convert to `on:click` for Svelte conventions). If you must retain the `<div>`, add an `onkeydown` event handler that triggers the `handleClick(d)` function when Enter or Space keys are pressed to support keyboard-only users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/components/TabsMenu.svelte`:
- Line 43: The TabsMenu component unconditionally applies the ui.selector.tabs
style at line 47, preventing it from being used with the ui.dialog style
required by the collection selector. Add a `style` prop parameter to the
TabsMenu component that allows callers to customize the styling. Modify the
styling logic where ui.selector.tabs is currently hardcoded to use this prop
value instead, with ui.selector.tabs as the default fallback when the prop is
not provided. This will enable the `/collections` page to pass ui.dialog as the
style prop when needed while maintaining backward compatibility with existing
usages.
In `@src/routes/collections/`+page.svelte:
- Around line 27-37: The `tabMenuActive` variable is hardcoded to
`Layout.Single` but the code below (showSinglePane, showSideBySide,
showVerseByVerse checks) determines which layouts are actually enabled. If
Layout.Single is disabled, this creates an invalid initial state. Instead of
hardcoding Layout.Single, initialize `tabMenuActive` by checking which layout is
actually enabled first using the boolean flags that are already being computed
(showSinglePane, showSideBySide, showVerseByVerse). Use a fallback logic to
select the first available enabled layout rather than assuming Layout.Single is
always valid.
---
Nitpick comments:
In `@src/lib/components/CollectionList.svelte`:
- Around line 40-47: Replace the `<div role="button">` element with a native
`<button>` element to improve keyboard accessibility. Remove the `role="button"`
attribute since native buttons have built-in keyboard support for Enter and
Space keys. Keep the existing `onclick` handler (or convert to `on:click` for
Svelte conventions). If you must retain the `<div>`, add an `onkeydown` event
handler that triggers the `handleClick(d)` function when Enter or Space keys are
pressed to support keyboard-only users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b95f2f51-d909-4b73-b125-bde8bc9ca6f1
📒 Files selected for processing (10)
src/lib/components/CollectionList.sveltesrc/lib/components/CollectionSelector.sveltesrc/lib/components/LayoutOptions.sveltesrc/lib/components/Sidebar.sveltesrc/lib/components/TabsMenu.sveltesrc/lib/data/stores/view.tssrc/routes/+layout.sveltesrc/routes/collections/+page.sveltesrc/routes/contents/[id]/+page.sveltesrc/routes/text/+page.svelte
💤 Files with no reviewable changes (3)
- src/lib/data/stores/view.ts
- src/lib/components/CollectionSelector.svelte
- src/routes/+layout.svelte
| let tabMenuActive = $state(Layout.Single); | ||
|
|
||
| // values of selectedLayouts before user makes changes | ||
| const restoreDocSets = JSON.stringify($selectedLayouts); | ||
|
|
||
| // ToDo: If showSinglePane false, provide first availible visible option instead | ||
| const showSinglePane = !!scriptureConfig.layouts?.find((x) => x.mode === Layout.Single) | ||
| ?.enabled; | ||
| const showSideBySide = !!scriptureConfig.layouts?.find((x) => x.mode === Layout.Two)?.enabled; //Not yet implemented | ||
| const showVerseByVerse = !!scriptureConfig.layouts?.find((x) => x.mode === Layout.VerseByVerse) | ||
| ?.enabled; //Not yet implemented |
There was a problem hiding this comment.
Initialize the active tab from enabled/current layout instead of hardcoding Layout.Single.
Line 27 always defaults to Layout.Single, but Lines 33-37 allow Single to be disabled. In that state, getSelectedLayout() (Line 39+) can resolve an invalid/empty selection and Line 51 can overwrite $refs.docSet with an empty value.
Proposed fix
- let tabMenuActive = $state(Layout.Single);
-
- // values of selectedLayouts before user makes changes
- const restoreDocSets = JSON.stringify($selectedLayouts);
-
- // ToDo: If showSinglePane false, provide first availible visible option instead
- const showSinglePane = !!scriptureConfig.layouts?.find((x) => x.mode === Layout.Single)
- ?.enabled;
- const showSideBySide = !!scriptureConfig.layouts?.find((x) => x.mode === Layout.Two)?.enabled; //Not yet implemented
- const showVerseByVerse = !!scriptureConfig.layouts?.find((x) => x.mode === Layout.VerseByVerse)
- ?.enabled; //Not yet implemented
+ // values of selectedLayouts before user makes changes
+ const restoreDocSets = JSON.stringify($selectedLayouts);
+
+ const showSinglePane = !!scriptureConfig.layouts?.find((x) => x.mode === Layout.Single)?.enabled;
+ const showSideBySide = !!scriptureConfig.layouts?.find((x) => x.mode === Layout.Two)?.enabled; //Not yet implemented
+ const showVerseByVerse = !!scriptureConfig.layouts?.find((x) => x.mode === Layout.VerseByVerse)?.enabled; //Not yet implemented
+
+ const enabledModes = [
+ showSinglePane && Layout.Single,
+ showSideBySide && Layout.Two,
+ showVerseByVerse && Layout.VerseByVerse
+ ].filter(Boolean) as Layout[];
+
+ let tabMenuActive = $state(
+ enabledModes.includes($layout.mode) ? $layout.mode : (enabledModes[0] ?? Layout.Single)
+ );
@@
function handleOk() {
const selectedLayout = getSelectedLayout();
+ if (!selectedLayout.primaryDocSet) return;
$refs.docSet = selectedLayout.primaryDocSet;
$layout = selectedLayout;
goto(resolve(`/`));
}Also applies to: 39-43, 50-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/collections/`+page.svelte around lines 27 - 37, The
`tabMenuActive` variable is hardcoded to `Layout.Single` but the code below
(showSinglePane, showSideBySide, showVerseByVerse checks) determines which
layouts are actually enabled. If Layout.Single is disabled, this creates an
invalid initial state. Instead of hardcoding Layout.Single, initialize
`tabMenuActive` by checking which layout is actually enabled first using the
boolean flags that are already being computed (showSinglePane, showSideBySide,
showVerseByVerse). Use a fallback logic to select the first available enabled
layout rather than assuming Layout.Single is always valid.
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
Not sure if this is in scope or if a separate issue should be opened, but changing the collection changes the interface font. (repro: select ASV).
| {#if mode === Layout.Single} | ||
| <SinglePaneIcon | ||
| color="black" | ||
| /><!--From what I can tell, the icons seem to be hardcoded to black in the native app, although I might just be missing the correct color setting to change--> |
There was a problem hiding this comment.
Did you try using the black theme in the native app?? The icons were white on my end at least.
There was a problem hiding this comment.
I can now see that they aren't hardcoded to black, but I can't figure out what setting does affect them. I tried every color I could find in Appearance->Colors->Main Colors that was dark in Normal and Sepia and light in Dark, changed the color in Sepia to some bright, flashy color, and then build and installed the Android app and found that the icons were still dark in normal and sepia modes. Do you have any idea what the color setting for them is?
There was a problem hiding this comment.
Have you tried inspecting the CSS through USB debugging?
There was a problem hiding this comment.
I can't figure out how to inspect the part of the screen that has the selector at the top with the icons. The only things that show up in the dev tools inspection are the tabs themselves for single pane and verse on image (I have the two-pane view disabled right now) and several unrendered pages of Scripture text.
| <!-- Single Pane --> | ||
| {#if layoutOption === Layout.Single} | ||
| <p class="py-2" style:color={$themeColors['LayoutTitleColor']}> | ||
| <p class="py-2 font-bold" style:color={$themeColors['LayoutTitleColor']}> |
There was a problem hiding this comment.
On larger screens, this "header" is all the way to the left, instead of aligned with the sort-of-centered , which doesn't look all that great. I'd recommend removing the centering from and moving it to LayoutOptions instead. Then you have slightly more control over the 's sizing across multiple places where you are using it (i.e. the dropdown for side by side or verse by verse). The selectors for side by side and verse by verse can then also be size-constrained and centered in the same stroke.
There was a problem hiding this comment.
What would you recommend removing the centering from and moving to LayoutOptions? Or were you suggesting moving the "header" from LayoutOptions to somewhere else?
There was a problem hiding this comment.
This is probably out of scope, but can you add a little padding around this? It turns out a bit cramped.
|
Changing the collection changes the interface font on the native app as well, so I think it's intended behavior. |
|
Also, I thought that the 2-pane feature and verse on image were both currently disabled. How did you access them in the collections page? |
I stand corrected. Please disregard lol. |
I manually changed the relevant values in config.ts |
Ah. I have not yet implemented those features at all, so they have not been properly styled. I'll get to work on those. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/CollectionList.svelte (1)
40-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a real button for collection row actions (keyboard access is currently broken).
Line 40 uses a
divwithonclickandrole="button"but no keyboard handler/tab focus support. This blocks keyboard activation for collection changes.Suggested fix
- <div - onclick={() => handleClick(d)} + <button + type="button" + onclick={() => handleClick(d)} style:background-color={selectedLayouts?.id === d.id ? $themeColors['LayoutItemSelectedBackgroundColor'] : ''} class="flex justify-between layout-item-block rounded-none" - role="button" > @@ - </div> + </button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/CollectionList.svelte` around lines 40 - 47, Replace the div element at line 40 with a real button element to restore keyboard accessibility. The current div has onclick handler with handleClick(d) and role="button" but lacks keyboard support. Convert the div to a button element, remove the role="button" attribute since a real button has built-in keyboard support, preserve all the style bindings (background-color), class names (flex, justify-between, layout-item-block, rounded-none), and the onclick handler with handleClick(d) to maintain the current functionality while enabling keyboard navigation and activation via Enter/Space keys.
♻️ Duplicate comments (1)
src/routes/collections/+page.svelte (1)
27-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize active layout from enabled modes before saving selection.
Line 27 still hardcodes
Layout.Single. If single-pane is disabled,getSelectedLayout()(Line 38 onward) can resolve an emptyprimaryDocSet, andhandleOk()(Line 49 onward) persists that empty value to$refs.docSet.Proposed minimal fix
- let tabMenuActive = $state(Layout.Single); + const enabledModes = [ + showSinglePane && Layout.Single, + showSideBySide && Layout.Two, + showVerseByVerse && Layout.VerseByVerse + ].filter(Boolean) as Layout[]; + + let tabMenuActive = $state( + enabledModes.includes($layout.mode) ? $layout.mode : (enabledModes[0] ?? Layout.Single) + ); @@ function handleOk() { const selectedLayout = getSelectedLayout(); + if (!selectedLayout.primaryDocSet) return; $refs.docSet = selectedLayout.primaryDocSet; $layout = selectedLayout; goto(resolve(`/`)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/collections/`+page.svelte around lines 27 - 53, The tabMenuActive variable is hardcoded to Layout.Single, which can cause getSelectedLayout() to return an empty primaryDocSet if single-pane is disabled, and handleOk() will then persist this invalid value to $refs.docSet. Instead of hardcoding Layout.Single, initialize tabMenuActive by selecting the first enabled layout mode from the available options, checking showSinglePane, showSideBySide, and showVerseByVerse flags in order to determine which layout should be active by default. This ensures that getSelectedLayout() will always have a valid enabled layout to work with.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/components/TabsMenu.svelte`:
- Line 78: Remove the stray debug text "Hello" that is being rendered on line 78
in the TabsMenu.svelte component. This literal text is appearing in the
production UI and should be deleted entirely from the tab content rendering
logic.
---
Outside diff comments:
In `@src/lib/components/CollectionList.svelte`:
- Around line 40-47: Replace the div element at line 40 with a real button
element to restore keyboard accessibility. The current div has onclick handler
with handleClick(d) and role="button" but lacks keyboard support. Convert the
div to a button element, remove the role="button" attribute since a real button
has built-in keyboard support, preserve all the style bindings
(background-color), class names (flex, justify-between, layout-item-block,
rounded-none), and the onclick handler with handleClick(d) to maintain the
current functionality while enabling keyboard navigation and activation via
Enter/Space keys.
---
Duplicate comments:
In `@src/routes/collections/`+page.svelte:
- Around line 27-53: The tabMenuActive variable is hardcoded to Layout.Single,
which can cause getSelectedLayout() to return an empty primaryDocSet if
single-pane is disabled, and handleOk() will then persist this invalid value to
$refs.docSet. Instead of hardcoding Layout.Single, initialize tabMenuActive by
selecting the first enabled layout mode from the available options, checking
showSinglePane, showSideBySide, and showVerseByVerse flags in order to determine
which layout should be active by default. This ensures that getSelectedLayout()
will always have a valid enabled layout to work with.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f45b9e16-abf3-45f7-a588-123ca4199bf5
📒 Files selected for processing (3)
src/lib/components/CollectionList.sveltesrc/lib/components/TabsMenu.sveltesrc/routes/collections/+page.svelte
Replaced collection selector modal with collections route
Also implement the buttons used for navigation out of the collection selection route
Also fix issue with current collection not being selected in the collection selector.
Work started on a modal that allows the user to select a collection for one of the selections for verse-by-verse or double pane.
Selecting a collection for verse-by-verse that is already selected now changes the duplicate to the first collection that hasn't already been selected.
TheNonPirate
force-pushed
the
update/collection-as-route
branch
from
f9540c2 to
b190c3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/components/CollectionModal.svelte`:
- Around line 58-60: The filter operation at line 58 and line 75 assigns
undefined to $selectedLayouts.sideBySide[i] and $selectedLayouts.sideBySide[1]
when no unused collection is available, which causes crashes later when
accessing .id or .image properties. Before assigning filter(...)[0], check that
the filter result array has at least one element and only perform the assignment
if a valid element exists, otherwise skip the assignment or handle it
appropriately to prevent undefined values from entering the selectedLayouts
state.
In `@src/lib/components/LayoutOptions.svelte`:
- Around line 114-117: The modal payload passed to modal.open for
ModalType.Collection does not match the contract expected by
CollectionModal.showModal, which requires a showBlank property. In the
modal.open call around line 114-117, add the missing showBlank property with a
value of false to the payload object alongside the existing type and number
properties, ensuring the dispatcher receives the complete payload structure that
showModal expects.
- Around line 109-113: The interactive elements with the class layout-item-block
that suppress accessibility checks are non-focusable divs with onclick handlers,
preventing keyboard users from accessing them. Replace both instances of these
div elements with semantically appropriate button elements instead of divs,
remove the svelte-ignore comments since buttons are natively accessible, and
ensure the onclick handlers are properly converted to button click handlers.
This will make the collection selectors keyboard-accessible without needing to
suppress accessibility warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1940be3f-b91a-4b78-afbf-4e4a3a9c702e
📒 Files selected for processing (11)
src/lib/components/CollectionList.sveltesrc/lib/components/CollectionModal.sveltesrc/lib/components/CollectionSelector.sveltesrc/lib/components/LayoutOptions.sveltesrc/lib/components/Sidebar.sveltesrc/lib/components/TabsMenu.sveltesrc/lib/data/stores/view.tssrc/routes/+layout.sveltesrc/routes/collections/+page.sveltesrc/routes/contents/[id]/+page.sveltesrc/routes/text/+page.svelte
💤 Files with no reviewable changes (1)
- src/lib/components/CollectionSelector.svelte
🚧 Files skipped from review as they are similar to previous changes (6)
- src/routes/contents/[id]/+page.svelte
- src/routes/text/+page.svelte
- src/lib/components/Sidebar.svelte
- src/routes/collections/+page.svelte
- src/lib/data/stores/view.ts
- src/lib/components/CollectionList.svelte
| <!-- svelte-ignore a11y_click_events_have_key_events --> | ||
| <!-- svelte-ignore a11y_no_static_element_interactions --> | ||
| <div | ||
| class="flex justify-between layout-item-block rounded-none cursor-pointer" | ||
| onclick={() => { |
There was a problem hiding this comment.
Use keyboard-accessible controls for collection selectors.
Line 109 and Line 157 suppress accessibility checks, but the interactive element is still a non-focusable <div onclick>. Keyboard users cannot reliably open these selectors.
Suggested fix
- <div
- class="flex justify-between layout-item-block rounded-none cursor-pointer"
- onclick={() => {
+ <button
+ type="button"
+ class="flex w-full justify-between layout-item-block rounded-none cursor-pointer text-left"
+ onclick={() => {
modal.open(ModalType.Collection, {
type: 'double-pane',
number: i
});
}}
- >
+ >
...
- </div>
+ </button>Also applies to: 157-161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/components/LayoutOptions.svelte` around lines 109 - 113, The
interactive elements with the class layout-item-block that suppress
accessibility checks are non-focusable divs with onclick handlers, preventing
keyboard users from accessing them. Replace both instances of these div elements
with semantically appropriate button elements instead of divs, remove the
svelte-ignore comments since buttons are natively accessible, and ensure the
onclick handlers are properly converted to button click handlers. This will make
the collection selectors keyboard-accessible without needing to suppress
accessibility warnings.
2 participants
In preparation for #593, the collection selector was changed from a modal to a route to better match the native app. In the process, the styling was also fixed, so it fixes #1002.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation