-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Placeholder Inspector Sidebar #141
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
Is there a reason for the |
struct InspectorSidebar: View { | ||
@ObservedObject var workspace: WorkspaceDocument | ||
var windowController: NSWindowController | ||
@State private var selection: Int = 0 |
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 using types like Int
or String
etc, which are really generic types.
Here we could create a new enum
, I think the logic will end up being much cleaner and easier to read
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 copied what was done in the navigator sidebar. I do agree though. Although it might end up having to be somewhat dynamic as we allow extensions to add new sidebar tabs.
|
||
var body: some View { | ||
HStack(spacing: 10) { | ||
icon(systemImage: "doc", title: "File Inspector", id: 0) |
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.
if we use that enum I was talking about here it would end up being like:
icon(systemImage: "doc", title: "File Inspector", id: .fileInspector)
workspace: workspace) | ||
.frame(maxWidth: .infinity, maxHeight: .infinity) | ||
InspectorSidebar(workspace: workspace, windowController: windowController) | ||
.frame(minWidth: 250, maxWidth: .infinity, maxHeight: .infinity) |
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.
using fixed values worries me a little bit... maybe 250 it's low and not that much but we should definitely put this value inside a constant that maybe is injected upon initalization?
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.
One thing I would do before merging this is creating a module for the InspectorSidebar
and a SnapshotTest
There are other files that need to be modules as well that are not. This was meant to just be a placeholder for later. I'd like to create a separate issue to move other things into modules as well like NavigationSidebar, Breadcrumbs, Statusbar etc. Doing this work now is out of scope of this PR. I will merge this in and we can always revise later. |
# Description This PR adds a `contentInsets` parameter that lets users set an inset object for the editor. It also updates `STTextView` to be able to set the left inset correctly. When left `nil` the scroll view will use automatic insets. This also doesn't override the overscroll option, but rather adds the overscroll after the insets are applied. # Related Issues - [Discord discussion](https://canary.discord.com/channels/951544472238444645/954084547694325861/1075085722211581952) - [STTextView commit](krzyzanowskim/STTextView@06d8f33) # Screenshots Adding top and left insets: <img width="741" alt="Screenshot 2023-02-14 at 3 09 57 PM" src="https://user-images.githubusercontent.com/35942988/218863914-8869e185-cd21-45a7-a7aa-58387ea778ea.png"> Adding left and bottom insets (scroller is also inset): <img width="835" alt="Screenshot 2023-02-14 at 3 10 15 PM" src="https://user-images.githubusercontent.com/35942988/218863899-99d2b124-fc96-418a-9869-d6897a7baea4.png">
No description provided.