-
Notifications
You must be signed in to change notification settings - Fork 0
Refresh backlinks regardless of whether editor content is stale #859
Conversation
After chatting to @gordonbrander we think it's more sensible to hold off merging this and split apart backlink refresh from editor content refresh. |
eb9252e
to
e3253c8
Compare
xcode/Subconscious/Shared/Components/Detail/MemoEditorDetail.swift
Outdated
Show resolved
Hide resolved
xcode/Subconscious/Shared/Components/Detail/MemoEditorDetail.swift
Outdated
Show resolved
Hide resolved
xcode/Subconscious/Shared/Components/Detail/MemoEditorDetail.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple notes
xcode/Subconscious/Shared/Components/Detail/MemoEditorDetail.swift
Outdated
Show resolved
Hide resolved
xcode/Subconscious/Shared/Components/Detail/MemoEditorDetail.swift
Outdated
Show resolved
Hide resolved
xcode/Subconscious/Shared/Components/Detail/MemoViewerDetailView.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direction looks good to me, but I have a few requests.
xcode/Subconscious/Shared/Components/Common/Profile/UserProfileView.swift
Outdated
Show resolved
Hide resolved
xcode/Subconscious/Shared/Components/Detail/MemoEditorDetail.swift
Outdated
Show resolved
Hide resolved
xcode/Subconscious/Shared/Components/Detail/MemoEditorDetail.swift
Outdated
Show resolved
Hide resolved
@@ -11,7 +11,6 @@ import Foundation | |||
struct MemoEditorDetailResponse: Hashable { | |||
var saveState: SaveState | |||
var entry: MemoEntry | |||
var backlinks: [EntryStub] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this struct only carries an entry now, should we just respond with a MemoEntry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still want to attach saveState
here, so the struct makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, missed that. You're right.
let model = prepareLoadDetail(state) | ||
return Update(state: model, fx: fx) | ||
var model = prepareLoadDetail(state) | ||
model.address = address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address must be set on model to fetch backlinks as early as possible.
c13da83
to
68867aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Would suggest playtesting since this touches the editor:
- Creating a note
- Deleting a note
- Renaming a note
- Creating a new note A, linking off to another new note B from it, following link to create B... does A show up in the related notes section?
- Opening the same note multiple times, changing the top one, paging back to the lower one. What happens?
23f6afb
to
6921248
Compare
Fixes #857
Changes
.appear
DataService.readMemoDetail
andDataService.readEditorMemoDetail
are both much simpler due to the introduction ofDataService.readMemoBacklinks
app
down to the viewer and editor so they can replay select actions.fromAppAction
pattern