-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
"Open note on server" menu item #7477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
contributor
wants to merge
2
commits into
TriliumNext:main
Choose a base branch
from
contributor:open-note-on-server
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+32
−1
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The
disabled={!syncServerHost}prop is redundant here. TheCommandItemis only rendered ifsyncServerHostis truthy due to the conditional rendering on line 87. In that case,!syncServerHostwill always evaluate tofalse. You can remove this prop for cleaner and more maintainable code.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.
@gemini-code-assist it is redundant, but I would still keep
disabledattribute to be replaced with potential another check - for notes that cannot be opened on server. I haven't identified if there are such notes - notes like Options and User Guide are still openable.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 understand your intention to keep the
disabledattribute for potential future checks. However, the currentdisabled={!syncServerHost}is still redundant.Here's why:
CommandItemon line 88 is only rendered ifsyncServerHostis truthy (due to(syncServerHost && isElectron)on line 87).syncServerHostis truthy, then!syncServerHostwill always evaluate tofalse.disabledprop will effectively always bedisabled={false}when thisCommandItemis visible.If you plan to add more complex disabling logic in the future (e.g., based on the note type), you would modify this
disabledprop anyway. Removing the currently redundantdisabled={!syncServerHost}now would simplify the code without hindering future enhancements, as you'd be adding the new logic to a clean slate. The currentdisabled={!syncServerHost}doesn't serve as a useful placeholder for future, more complex disabling conditions.Uh oh!
There was an error while loading. Please reload this page.
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.
@contributor I'd go here with gemini, had the same argument in my head. I can't imagine why the server shouldn't support a note type that is supported on desktop that would be instantly a Bug.
Uh oh!
There was an error while loading. Please reload this page.
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.
For example, folder view - a note that shows content of a local folder. You are likely won't have such note on server due to security reason and file system path differences.
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.
@contributor I think even syncing up a potential folder view would be beneficial and it then would just open with error, it is only configured for X, Y, Z. Because the structure reminds the user oh wait I might have to open it up on my Computer.
Uh oh!
There was an error while loading. Please reload this page.
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.
@contributor however I might see an edge case where you'd be right. Assume you upload images that you dont want to be shared on your trilium server instance. Then not having them synced up, maybe if there would be an option to hide this from the server, that would be beneficial, making the approach ofc worth keep having.
I would think this would resolve the comments then.
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.
Yes, local only (not synced) notes too. Evernote had it 15+ years ago. And if you don't trust the trilium sync server - that could be useful feature (even more true if they turn trilium to multi-user server)