-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Breadcrumbs Navigation #44
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
Conversation
(cherry picked from commit d4d14bf)
Colors would be nice, also in the sidebar. I think we include monochromatic / color icon toggle as a setting to be applied to icons in the sidebar and breadcrumb bar. I will create an issue. Don't worry about that in this PR. |
@austincondiff the last todo of this PR (navigating using the breadcrumbs) will be added later - still have to figure out a lot of things in this regard. I would merge this as is for now. |
@MarcoCarnevali please review 😊 |
|
||
var title: String | ||
var image: String | ||
var color: Color |
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.
are these (title,image,color) values going to change? Should these be public accessibles?
If not I would set those as privates and change it to let
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'll change them to let
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.
and they're private now
struct BreadcrumbsView: View { | ||
|
||
@ObservedObject var workspace: WorkspaceDocument | ||
var file: WorkspaceClient.FileItem |
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 the file
could just be a let
?
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.
true
|
||
private func fileInfo() { | ||
guard let projName = workspace.workspaceClient?.folderURL()?.lastPathComponent else { return } | ||
var components = file.url.pathComponents.split(separator: projName).last! |
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.
we should try to avoid force casts... I think we can just put this inside the guard so we don't have to force it
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.
good point 😄
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.
like
guard let projName = workspace.workspaceClient?.folderURL()?.lastPathComponent,
let components = file.url.pathComponents.split(separator: projName).last else { return }
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.
yeah this should work great!
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.
let components
will be changed to var components
since it needs to be mutable.
@@ -9,14 +9,19 @@ import Combine | |||
import Foundation | |||
|
|||
public struct WorkspaceClient { | |||
|
|||
public var folderURL: () -> URL? |
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.
honestly I wouldn't add this API. The folderURL
is injected while initializing the client itself, so the consumer already has a reference of the directory somewhere
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.
Where is the question though. couldn't find a reference
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.
WorkspaceDocument
is the class responsible to build the WorkspaceClient
folderURL: url, |
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.
this means that WorkspaceDocument
has a reference of the folderURL
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.
ok I added a reference var folderURL: URL?
to WorkspaceDocument
Divider() | ||
} | ||
.onAppear { | ||
fileInfo() |
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.
Why you get fileInfo in onAppear
instead of init
?
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.
Because the @State
seems to get initialized after .init()
and so the names are blank
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.
All right. Just feel weird do that in onAppear
, it means every time the window is active it'll be called.
This PR adds Breadcrumbs Navigation above the editor
This is still WIP. It already shows the right path but navigating is not yes supported.
Also colors would be nice I think
Make navigation possible by clicking on a path component (just like in Xcode)(saved for later, investigation ongoing)