Skip to content
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.

Sign up for GitHub

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

Update FileSharePasswordView #40

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update FileSharePasswordView #40

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 32 additions & 26 deletions JamfSync/UI/FileSharePasswordView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ struct FileSharePasswordView: View {

@Binding var fileShareDp: FileShareDp?
@Binding var canceled: Bool
@State var rwUsername = ""

@State var saveInKeychain = true
@State var password = ""
let keychainHelper = KeychainHelper()
Expand All @@ -19,33 +21,35 @@ struct FileSharePasswordView: View {
.font(.title)
Text("Enter the password for \(fileShareDp?.selectionName() ?? ""): \(fileShareDp?.address ?? "")")
.padding(.bottom)

HStack {
VStack(alignment: .trailing) {
Text("Password:")
.frame(height: 16)
}
VStack {
HStack {
SecureField(text: $password, prompt: Text("Password")) {
Text("Title")
}
Toggle(isOn: $saveInKeychain) {
Text("Save in Keychain")
}
}
.frame(height: 16)
}

Form {
Section(header: Text("Account:") // Label
.font(.headline)
.padding(EdgeInsets(top: 0, leading: 0, bottom: 0, trailing: 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? I would think removing this padding call would have the same results.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if it is necessary, can you achieve the same thing by just calling .padding(0) so you don't have to create the new object each time it's redrawn?

) {
TextField("", text: $rwUsername, prompt: Text(""))
.disabled(true)
.frame(width: 420, height: 30, alignment: .leading)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best to avoid frames altogether if you can, but if not, then just specify the parts of the frame that you really need (like height maybe).

.padding(.leading, 10)
.padding(.trailing, 20)
}
.textCase(nil)

Section(header: Text("Password:") // Label
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The horizontal spacing is messed up around this.

.font(.headline)
.padding(EdgeInsets(top: 0, leading: 0, bottom: 0, trailing: 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? I would think removing this padding call would have the same results.

) {
SecureField("", text: $password, prompt: Text(""))
.frame(width: 420, height: 30, alignment: .leading)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best to avoid frames altogether if you can, but if not, then just specify the parts of the frame that you really need (like height maybe).

}
.textCase(nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil is the default, so this can be removed.

}

HStack {
Spacer()
Button("Cancel") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cancel button was removed and it's still necessary.

canceled = true
dismiss()
Toggle(isOn: $saveInKeychain) {
Text("Save in Keychain")
}
.keyboardShortcut(.cancelAction)
.padding([.top, .trailing])
.padding(.leading, 80)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to just use default padding here. So .padding(.leading).

Spacer()
Button("OK") {
canceled = false
fileShareDp?.readWritePassword = password
Expand All @@ -54,13 +58,15 @@ struct FileSharePasswordView: View {
dismiss()
}
.keyboardShortcut(.defaultAction)
.padding(.top)
Spacer()
.padding(.trailing, 80)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to just use default padding here. So .padding(.trailing).

}
.padding([.bottom], 10)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best to just use the default padding if possible. And since it's only one padding item, I don't think you need to put it in an array. So try just .padding(.bottom).

}
.onAppear() {
saveInKeychain = UserSettings.shared.saveDistributionPointPwInKeychain
password = fileShareDp?.readWritePassword ?? ""
rwUsername = "\(fileShareDp?.readWriteUsername ?? "")"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be replaced by rwUsername = fileShareDp?.readWriteUsername ?? ""

if rwUsername == "'''" { rwUsername = ""}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this? Also, nitpicky, but I would prefer that the rwUsername = "" is on its own line, and at the very least, the spacing after the = and before the } should be consistent.

}
.padding()
.frame(width: 600)
Expand Down