Implement songbooks#972
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds end-to-end support for songs as a native book type: converting songs USFM with chapter-verse preprocessing, routing songs navigation to a dedicated page at ChangesSongs Book Type Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
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/BookSelector.svelte (1)
97-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
navigateToTextin the URL-navigation branch.This branch now does two navigations:
navigateToText(...)(which goes to/textand records history) and thennavigateToUrl(...). That creates an unintended intermediate route jump and extra history entry for songs/quiz selection.Use a state-only reference update (
refs.set(...)or a dedicated helper) beforenavigateToUrl(...)instead ofnavigateToText(...).Suggested fix
- await navigateToText({ + await refs.set({ docSet: $refs.docSet, - collection: $refs.collection, book: $nextRef.book, chapter: $nextRef.chapter, verse: $nextRef.verse }); navigateToUrl({ collection: $refs.collection, book: text, url: url });🤖 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/BookSelector.svelte` around lines 97 - 112, The branch currently calls navigateToText(...) then navigateToUrl(...), causing an unwanted intermediate route and history entry; replace the navigateToText call with a state-only update using refs.set(...) or the existing refs helper so you only update $nextRef.book/$nextRef.chapter/$nextRef.verse (or call refs.set({ book: text, chapter: '1', verse: '' })) and then call navigateToUrl({ collection: $refs.collection, book: text, url }); remove the await navigateToText(...) call and ensure only the state is mutated before navigateToUrl to avoid pushing /text into history.
🤖 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 `@convert/convertBooks.ts`:
- Around line 770-772: The current songs branch in convertBooks.ts
unconditionally appends "\v 1" after every section header (the block where
book.type === 'songs' and content is modified), causing duplicate verse markers;
change the replacement so it only inserts "\v 1" when the header line is not
already immediately followed by a verse marker (i.e., ensure the match checks
the following line does not start with "\v"). Locate the snippet that does
content = content.replace(/^\\s.*$/gm, '$&\n\\v 1') and update the regex or
replacement logic to include a negative lookahead (or a small callback that
inspects the next line) so headers already followed by "\v" are left unchanged.
In `@src/lib/components/ChapterSelector.svelte`:
- Around line 21-26: The derived value showVerseSelector currently treats an
unresolved book (where ?.type is undefined) as not 'songs' and thus true; update
the $derived expression to first resolve the current book into a variable and
explicitly check its existence before comparing its type (e.g., ensure the found
book object exists and then evaluate bookObj.type !== 'songs'), using
$userSettingsOrDefault, scriptureConfig, $refs.collection and book to locate the
book; keep the final result cast to boolean so unresolved/missing book yields
false instead of true.
In `@src/lib/components/Sidebar.svelte`:
- Around line 81-82: The divider is shown because showSongbooks is hardcoded
true while the actual songbook <li> is commented out; update the Sidebar.svelte
logic so the separator only appears when a real songbook menu item will render.
Either set showSongbooks to the real feature flag or runtime condition used
elsewhere (remove the hardcoded const showSongbooks = true) or remove
showSongbooks from the divider condition so the separator uses the same
visibility expression as the songbook <li> (match the condition used for
rendering the commented-out menu item); apply the same fix for the duplicate
instance around the block referenced at lines ~217-225.
In `@src/routes/songs/`[collection]/[id]/+page.svelte:
- Around line 19-31: The parsing logic for data.titleData and data.numberData is
fragile because it only splits on '\r\n'; update the split to use a
cross-platform regex like /\r?\n/ and filter out empty lines before splitting
each line on '\t', so that songsByTitle and songsByNumber are built correctly
for LF-only or CRLF files; keep existing checks for separatedValue.length === 2
and push objects with number and title as before.
- Around line 45-50: completeNavigation currently reads $refs.collection and
$refs.book which can be stale on deep links; update the navigateToText call to
use the route loader values (data.collection and data.songId) as the source of
truth. Replace references to $refs.collection and $refs.book inside
completeNavigation/navigateToText invocation with data.collection and
data.songId (keep songNumber and $nextRef.verse as before) so navigation is
deterministic even when the stores are not initialized.
In `@src/routes/songs/`[collection]/[id]/+page.ts:
- Around line 23-47: The current single try/catch around both fetches causes one
failure to skip the other; split the work so title and number are fetched
independently (either wrap the title fetch block that sets
titleKey/titleUrl/titleResponse/titleData in its own try/catch and do the same
for the numberKey/numberUrl/numberResponse/numberData, or replace both with a
Promise.allSettled that fetches both URLs and assigns titleData/numberData only
for fulfilled results), log errors per asset instead of rethrowing so a failure
in one asset does not wipe the other’s data.
---
Outside diff comments:
In `@src/lib/components/BookSelector.svelte`:
- Around line 97-112: The branch currently calls navigateToText(...) then
navigateToUrl(...), causing an unwanted intermediate route and history entry;
replace the navigateToText call with a state-only update using refs.set(...) or
the existing refs helper so you only update
$nextRef.book/$nextRef.chapter/$nextRef.verse (or call refs.set({ book: text,
chapter: '1', verse: '' })) and then call navigateToUrl({ collection:
$refs.collection, book: text, url }); remove the await navigateToText(...) call
and ensure only the state is mutated before navigateToUrl to avoid pushing /text
into history.
🪄 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: d37e2dc9-8329-491c-80c8-6c0bf03383dc
📒 Files selected for processing (9)
convert/convertBooks.tssrc/lib/components/BookSelector.sveltesrc/lib/components/ChapterSelector.sveltesrc/lib/components/Sidebar.sveltesrc/lib/icons/SongBookIcon.sveltesrc/lib/icons/index.tssrc/lib/types.tssrc/routes/songs/[collection]/[id]/+page.sveltesrc/routes/songs/[collection]/[id]/+page.ts
| const showVerseSelector = $derived($userSettingsOrDefault['verse-selection']) as boolean; | ||
| const showVerseSelector = $derived( | ||
| $userSettingsOrDefault['verse-selection'] && | ||
| scriptureConfig.bookCollections |
There was a problem hiding this comment.
A similar filter should be in BookSelector?
There was a problem hiding this comment.
The behavior in the native app is that the verse selector still shows up for the book selector, but not for the chapter selector. Do we want to keep it how it is to match the native app, or change it because it doesn't make much sense to still have the verse selector for songs?
There was a problem hiding this comment.
In my mind at least, it doesn't really make any sense to keep it, but on the other hand fidelity to the native app is still important... Thoughts @chrisvire ?
| const showNotes = !!config.mainFeatures['annotation-notes']; | ||
| const showHighlights = !!config.mainFeatures['annotation-highlights']; | ||
| const showPlans = scriptureConfig.plans?.plans.length; | ||
| const showSongbooks = true; |
There was a problem hiding this comment.
This appears to be incomplete.
There was a problem hiding this comment.
This (And the SongBookIcon) was part of some code in the previous attempt at songbooks. It added an icon in the menu that apparently could be used to go directly to the songbook. However, I can't find any setting in SAB that would cause such an icon to show up. Do you know of any such setting, or should I just remove this completely?
There was a problem hiding this comment.
I have no idea. That would be a question for @chrisvire
There was a problem hiding this comment.
I decided to go ahead and remove it. If it really is a setting that we need to support, I can reimpliment it.
TheNonPirate
force-pushed
the
feature/songbooks/859
branch
from
3ab317a to
cecf77f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/routes/songs/[collection]/[id]/+page.svelte (3)
20-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSong parsing is newline-format fragile.
The parser splits only on
'\r\n', so LF-only files ('\n') won't parse correctly. Use a cross-platform split like/\r?\n/and filter empty lines.🤖 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/songs/`[collection]/[id]/+page.svelte around lines 20 - 33, The parsing uses data.titleData.split('\r\n') and data.numberData.split('\r\n'), which fails on LF-only files; update both splits to use a cross-platform regex (e.g. /\r?\n/) and filter out empty lines before processing so blank lines are skipped; keep the rest of the logic that maps parts to songsByTitle and songsByNumber and still guard on separatedValue.length === 2.
46-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse route data as the source of truth for song navigation.
completeNavigationreads$refs.collectionand$refs.book, which can be stale on deep links or when stores aren't initialized. The loader already providesdata.collectionanddata.songId; use those for deterministic navigation.🤖 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/songs/`[collection]/[id]/+page.svelte around lines 46 - 54, completeNavigation currently reads from reactive stores $refs.collection and $refs.book which can be stale; change it to use the route loader values instead (data.collection and data.songId) so navigation is deterministic. Update completeNavigation to call navigateToText with docSet: $refs.docSet (if still valid) but replace collection: $refs.collection and book: $refs.book with collection: data.collection and chapter/verse derived from data.songId (or data.songId parsed into chapter and verse) and ensure you pass the existing $nextRef.verse; locate the function completeNavigation and the call to navigateToText to make this swap and handle parsing of data.songId if needed.
18-33: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winData processing should be moved to
$derivedor the loader.Parsing raw strings in an IIFE runs once at component initialization and isn't reactive to data changes. Either refactor to
$derivedfor reactivity or move parsing to+page.tsfor better separation of concerns.🤖 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/songs/`[collection]/[id]/+page.svelte around lines 18 - 33, Move the parsing logic out of the IIFE and into a reactive or loader context: either create derived stores that produce songsByTitle and songsByNumber from data.titleData and data.numberData (use Svelte's $derived or $: reactive statements to split by '\r\n' and '\t' so updates to data propagate), or perform the parsing in the page loader (+page.ts) and return parsed songsByTitle and songsByNumber from load so the component uses them directly; update references to songsByTitle/songsByNumber in the component and remove the IIFE to keep concerns separated and ensure reactivity.
🤖 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/routes/songs/`[collection]/[id]/+page.svelte:
- Line 36: Replace the static let tabs = [...] with a reactive derived store so
labels update when the translation store changes: import { derived } from
'svelte/store' and change to const tabs = derived(t, $t =>
[$t['Song_List_By_Number'], $t['Song_List_By_Title']]); update any template
usages from {tabs} to {$tabs} (or adjust code that expected an array
accordingly) so tab labels react to language changes.
---
Duplicate comments:
In `@src/routes/songs/`[collection]/[id]/+page.svelte:
- Around line 20-33: The parsing uses data.titleData.split('\r\n') and
data.numberData.split('\r\n'), which fails on LF-only files; update both splits
to use a cross-platform regex (e.g. /\r?\n/) and filter out empty lines before
processing so blank lines are skipped; keep the rest of the logic that maps
parts to songsByTitle and songsByNumber and still guard on separatedValue.length
=== 2.
- Around line 46-54: completeNavigation currently reads from reactive stores
$refs.collection and $refs.book which can be stale; change it to use the route
loader values instead (data.collection and data.songId) so navigation is
deterministic. Update completeNavigation to call navigateToText with docSet:
$refs.docSet (if still valid) but replace collection: $refs.collection and book:
$refs.book with collection: data.collection and chapter/verse derived from
data.songId (or data.songId parsed into chapter and verse) and ensure you pass
the existing $nextRef.verse; locate the function completeNavigation and the call
to navigateToText to make this swap and handle parsing of data.songId if needed.
- Around line 18-33: Move the parsing logic out of the IIFE and into a reactive
or loader context: either create derived stores that produce songsByTitle and
songsByNumber from data.titleData and data.numberData (use Svelte's $derived or
$: reactive statements to split by '\r\n' and '\t' so updates to data
propagate), or perform the parsing in the page loader (+page.ts) and return
parsed songsByTitle and songsByNumber from load so the component uses them
directly; update references to songsByTitle/songsByNumber in the component and
remove the IIFE to keep concerns separated and ensure reactivity.
🪄 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: 4f54c3c7-12b3-458d-9d83-bb2f5b033427
📒 Files selected for processing (9)
convert/convertBooks.tssrc/lib/components/BookSelector.sveltesrc/lib/components/ChapterSelector.sveltesrc/lib/components/Sidebar.sveltesrc/lib/icons/SongBookIcon.sveltesrc/lib/icons/index.tssrc/lib/types.tssrc/routes/songs/[collection]/[id]/+page.sveltesrc/routes/songs/[collection]/[id]/+page.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/icons/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/types.ts
- src/lib/components/Sidebar.svelte
- src/lib/components/BookSelector.svelte
- src/lib/components/ChapterSelector.svelte
- src/lib/icons/SongBookIcon.svelte
- convert/convertBooks.ts
- src/routes/songs/[collection]/[id]/+page.ts
|
I realized that SAB has a "Hide verse number 1 if it is directly after the chapter number" feature, which is enabled in the project that I'm testing but can be disabled. This means that my approach of inserting a verse number 1 at the beginning of the song in order to display the chapter number won't work because it can lead to the dummy verse number being displayed. |
TheNonPirate
force-pushed
the
feature/songbooks/859
branch
from
7b43833 to
8d9238d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ScriptureViewSofria.svelte`:
- Around line 2025-2028: The code mixes reactive $refs (e.g. $refs.book,
$refs.collection) with the non-reactive references object (references.book,
references.collection), causing inconsistent reads; update the two lines that
declare book and bookType to consistently use the same source (use
references.book and references.collection to match surrounding logic) and derive
bookType from scriptureConfig.bookCollections?.find(x => references.collection
=== x.id)?.books.find(b => references.book === b.id)?.type so all lookups
reference the same variables (references.book, references.collection, and
scriptureConfig.bookCollections).
🪄 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: d440dfdc-d712-425b-bc88-dc70bccac387
📒 Files selected for processing (9)
convert/convertBooks.tssrc/lib/components/BookSelector.sveltesrc/lib/components/ChapterSelector.sveltesrc/lib/components/ScriptureViewSofria.sveltesrc/lib/components/Sidebar.sveltesrc/lib/icons/SongBookIcon.sveltesrc/lib/icons/index.tssrc/routes/songs/[collection]/[id]/+page.sveltesrc/routes/songs/[collection]/[id]/+page.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/icons/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- convert/convertBooks.ts
- src/lib/components/BookSelector.svelte
- src/lib/icons/SongBookIcon.svelte
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
The indentation on the first line doesn't look right compared to the rest of the lines.
Native for comparison:
We may also want to horizontally center the container??
Also, Hark the Herald Angels Sing is missing a lot of the first letter of each line, this should probably be fixed.
Finally, there are some merge conflicts, so you'll need to rebase again.
|
I think the indentation issue is there on Scripture books as well, just not as obviously generally. In this example, the same general issue can be seen: I'm pretty sure the container is also not horizontally centered in Scripture books either. |
TheNonPirate
force-pushed
the
feature/songbooks/859
branch
from
a4994e3 to
7bd1d17
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
convert/convertBooks.ts (1)
505-529:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnresolved merge conflict markers break compilation.
Lines 505-509 contain Git merge conflict markers that cause syntax errors. The static analysis confirms the code cannot be parsed.
Based on the PR objectives (enable songs processing, handle story specially), resolve by:
- Removing the conflict markers
- Removing
case 'songs':(songs should now fall through to default for scripture processing)- Keeping
case 'story':beforedefault:at line 529 for the special page-to-chapter conversion in lines 888-896🔧 Proposed fix
switch (book.type) { -<<<<<<< HEAD - case 'songs': -======= - case 'story': ->>>>>>> f2db8b4 (Feature- Songbooks) case 'audio-only': case 'bloom-player': case 'undefined': break; case 'quiz': bookConverted = true; // ... quiz handling ... break; case 'story': default:🤖 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 `@convert/convertBooks.ts` around lines 505 - 529, Remove the Git conflict markers and the obsolete case branch so the switch compiles: delete the <<<<<<<, =======, and >>>>>>> markers and remove the case 'songs': clause so songs fall through to the existing default/scripture handling; ensure case 'story': remains present immediately before the default branch in the same switch (so story continues to trigger the special page-to-chapter conversion logic used by convertQuizBook/convertBooks), and verify break/return flow for case 'story' and subsequent cases is correct to avoid fall-through bugs.Source: Linters/SAST tools
🧹 Nitpick comments (1)
convert/convertBooks.ts (1)
394-395: 💤 Low valueUnused
bookTypeparameter.The
bookTypeparameter was added toapplyFiltersbut is never used in the function body. Either remove it or implement the intended conditional logic.🤖 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 `@convert/convertBooks.ts` around lines 394 - 395, The applyFilters function signature currently includes an unused bookType parameter; remove bookType from the applyFilters signature in convert/convertBooks.ts (and drop it from any callers) to eliminate the dead parameter, or if the intent was to filter by type implement the check inside applyFilters (e.g., accept bookType and early-return/filter items when their type doesn't match) and ensure all call sites pass the correct value; update the function named applyFilters and any callers to be consistent.
🤖 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.
Outside diff comments:
In `@convert/convertBooks.ts`:
- Around line 505-529: Remove the Git conflict markers and the obsolete case
branch so the switch compiles: delete the <<<<<<<, =======, and >>>>>>> markers
and remove the case 'songs': clause so songs fall through to the existing
default/scripture handling; ensure case 'story': remains present immediately
before the default branch in the same switch (so story continues to trigger the
special page-to-chapter conversion logic used by convertQuizBook/convertBooks),
and verify break/return flow for case 'story' and subsequent cases is correct to
avoid fall-through bugs.
---
Nitpick comments:
In `@convert/convertBooks.ts`:
- Around line 394-395: The applyFilters function signature currently includes an
unused bookType parameter; remove bookType from the applyFilters signature in
convert/convertBooks.ts (and drop it from any callers) to eliminate the dead
parameter, or if the intent was to filter by type implement the check inside
applyFilters (e.g., accept bookType and early-return/filter items when their
type doesn't match) and ensure all call sites pass the correct value; update the
function named applyFilters and any callers to be consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b65ef962-a59c-48af-87e1-b911a629b834
📒 Files selected for processing (9)
convert/convertBooks.tssrc/lib/components/BookSelector.sveltesrc/lib/components/ChapterSelector.sveltesrc/lib/components/ScriptureViewSofria.sveltesrc/lib/components/Sidebar.sveltesrc/lib/icons/SongBookIcon.sveltesrc/lib/icons/index.tssrc/routes/songs/[collection]/[id]/+page.sveltesrc/routes/songs/[collection]/[id]/+page.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/icons/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/components/BookSelector.svelte
- src/lib/components/Sidebar.svelte
- src/lib/components/ChapterSelector.svelte
- src/lib/components/ScriptureViewSofria.svelte
- src/lib/icons/SongBookIcon.svelte
- src/routes/songs/[collection]/[id]/+page.ts
- src/routes/songs/[collection]/[id]/+page.svelte
TheNonPirate
force-pushed
the
feature/songbooks/859
branch
from
6a675fb to
9ee59bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ScriptureViewSofria.svelte`:
- Around line 2035-2067: The song chapter number is rendered in the
chapter_label path but workspace.chapterNumText is left populated so
handleVerseLabel (used by verses_label) renders it again; after you append the
chapter number DOM node in the song branch of the chapter_label logic (both the
'drop-cap' branch that appends to workspace.paragraphDiv and the else branch
that appends to workspace.root), clear workspace.chapterNumText (or set a
dedicated flag like workspace.chapterNumRendered = true and check it in
handleVerseLabel) so handleVerseLabel will not double-render the chapter number.
🪄 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: ddf7e5bb-d67f-48c1-83e4-5822db0f384f
📒 Files selected for processing (6)
convert/convertBooks.tssrc/lib/components/BookSelector.sveltesrc/lib/components/ChapterSelector.sveltesrc/lib/components/ScriptureViewSofria.sveltesrc/routes/songs/[collection]/[id]/+page.sveltesrc/routes/songs/[collection]/[id]/+page.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/components/ChapterSelector.svelte
- convert/convertBooks.ts
- src/routes/songs/[collection]/[id]/+page.svelte
- src/lib/components/BookSelector.svelte
TheNonPirate
force-pushed
the
feature/songbooks/859
branch
from
643a8a1 to
c7c0e6a
Compare
|
I've updated it. |
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
One last nitpick. You don't have to make that change, but you do still have a merge conflict. Once the conflict is resolved I'm good to approve this.
|
This ended up being more complicated than I expected. I ended up going ahead and hiding the verse selector in the BookSelector component as well as in the ChapterSelector. That isn't exactly what the native app does, but it makes more sense, and the native app displayed the verse as "i" while the PWA was displaying it as a number or as no number at all, so it wasn't matching the native app anyway. I also found some problems relating to how I navigated to the song that caused the chapter number on the top of the screen to sometimes not update when moving between chapters. |
* Create new component for Songbook * add new route for Songbooks * create svelte page for individual songs and songbook menu * borrow icon until icon can be selected
Going to a songbook now has the correct top bar, but the song selection feature is not yet implemented.
One is for songs sorted by title, and one is for songs sorted by number.
Instead of inserting verse tags in conversion, which can cause problems for certain settings, insert chapter numbers in songs in ScriptureViewSofria.
Removed redundant truthy check, simplified src/routes/songs/[collection]/[id]/+page.ts data export, and removed some unneccessary derived variables
Cause verse selector to be hidden in the BookSelector component for songbooks. Fix some problems related to navigation in songbooks not updating chapter number properly
TheNonPirate
force-pushed
the
feature/songbooks/859
branch
from
4d102e4 to
a727675
Compare
|
It should be good now, but enough did change that it's probably worth another review. |
3 participants
Closes #859 and building on #881. This implements songbooks. When a songbook is selected, the app goes to a page that allows the user to select which song they want, sorted by number or title. Currently, the song's text is rendered the same as any scripture book, which means that it does not perfectly match the mobile app.
Summary by CodeRabbit
Release Notes
New Features
Changes