Fix: Display current note in BreadCrumbs component #324
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the display of the current note in the breadcrumbs component by ensuring the note's title is appended to the breadcrumbs and refines the display logic.
- Updated the BreadCrumbs component to conditionally append the current note based on availability.
- Refined breadcrumb link generation and display text logic to handle both string and object types.
- Simplified note hierarchy and parents assignment in the note service.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/presentation/pages/Note.vue | Updated binding logic to append the current note info to the breadcrumbs. |
| src/presentation/components/breadcrumbs/BreadCrumbs.vue | Modified navigation link generation and added type checking for breadcrumb content. |
| src/application/services/useNote.ts | Refactored async call in getNoteHierarchy and updated note parents assignment. |
Comments suppressed due to low confidence (1)
src/presentation/components/breadcrumbs/BreadCrumbs.vue:8
- [nitpick] The nested ternary operator used for rendering the breadcrumb label is hard to read. Consider refactoring this logic—perhaps extracting it to a computed property—to improve maintainability.
{{ parent.content ? typeof parent.content === 'string' ? parent.content : getTitle(parent.content) : '...' }}
There was a problem hiding this comment.
Pull Request Overview
This PR updates the note breadcrumbs display to include the current note title, ensuring that the title appears in the navigation path. Additionally, it refines the routing in the BreadCrumbs component and updates the note service to refresh the parents list after note creation.
- Updated Note.vue to conditionally append the current note’s id and title to the breadcrumbs.
- Refactored BreadCrumbs.vue to simplify the :to directive and use a computed helper for displaying parent titles.
- Adjusted useNote.ts to immediately update noteParents after creating a note.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/presentation/pages/Note.vue | Appends current note details to breadcrumbs conditionally. |
| src/presentation/components/breadcrumbs/BreadCrumbs.vue | Adjusts routing link generation and refines display logic. |
| src/application/services/useNote.ts | Directly assigns noteHierarchy and updates noteParents. |
| @@ -2,10 +2,10 @@ | |||
| <RouterLink | |||
| v-for="(parent, index) in displayedParents" | |||
| :key="index" | |||
| :to="{ path: parent.id ? `/note/${parent.id}` : '' }" | |||
| :to="`/note/${parent.id}`" | |||
There was a problem hiding this comment.
The updated :to directive always generates a URL using /note/${parent.id}. If parent.id is falsy (e.g., undefined), this may produce an unintended route like '/note/undefined'. Consider retaining a conditional check or default value to handle falsy parent.id values.
| :to="`/note/${parent.id}`" | |
| :to="`/note/${parent.id || 'unknown'}`" |
Copilot uses AI. Check for mistakes.
| const parentTitle = computed(() => { | ||
| return (content: string | null | OutputData) => { | ||
| if (content === null) { | ||
| return '...'; | ||
| } | ||
| if (typeof content === 'string') { | ||
| return content; | ||
| } | ||
|
|
||
| return getTitle(content); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
[nitpick] Defining parentTitle as a computed property that returns a helper function adds unnecessary complexity. It may be simpler and more direct to declare parentTitle as a standalone helper function.
| const parentTitle = computed(() => { | |
| return (content: string | null | OutputData) => { | |
| if (content === null) { | |
| return '...'; | |
| } | |
| if (typeof content === 'string') { | |
| return content; | |
| } | |
| return getTitle(content); | |
| }; | |
| }); | |
| function parentTitle(content: string | null | OutputData): string { | |
| if (content === null) { | |
| return '...'; | |
| } | |
| if (typeof content === 'string') { | |
| return content; | |
| } | |
| return getTitle(content); | |
| } |
Copilot uses AI. Check for mistakes.
| @@ -263,6 +260,7 @@ export default function (options: UseNoteComposableOptions): UseNoteComposableSt | |||
| */ | |||
| const noteCreated = await noteService.createNote(content, specifiedNoteTools, parentId); | |||
|
|
|||
| noteParents.value = (await noteService.getNoteById(noteCreated.id)).parents; | |||
There was a problem hiding this comment.
add comment explaining this statement, please
By this PR, in note's breadcrumbs also its title will be displayed
This resolves #323