-
Notifications
You must be signed in to change notification settings - Fork 53
[PM-10403] Added SSH key cipher item view #921
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #921 +/- ##
==========================================
+ Coverage 89.05% 89.10% +0.04%
==========================================
Files 654 656 +2
Lines 41057 41261 +204
==========================================
+ Hits 36562 36764 +202
- Misses 4495 4497 +2 ☔ View full report in Codecov by Sentry. |
…git push operation" This reverts commit 5de479516e28654bd0613ecd0eecc48b4b03ac1f.
No New Or Fixed Issues Found |
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.
👍🏻 Seems very reasonable so far!
@@ -137,7 +137,7 @@ struct AddEditItemView: View { | |||
BitwardenMenuField( | |||
title: Localizations.type, | |||
accessibilityIdentifier: "ItemTypePicker", | |||
options: CipherType.allCases, | |||
options: CipherType.allCases.filter { $0 != .sshKey }, |
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.
🤔 Instead of filtering here, could we possibly put a property on CipherType
to enumerate all the types that are addable by the mobile app? Just a thought, especially in case we add more types down the line that need to be filtered out in this sort of situation.
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 makes sense, I'll make the change.
// MARK: - SSHKeyItemState | ||
|
||
/// The state for an SSH key item. | ||
struct SSHKeyItemState: Equatable, Sendable { |
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 are super inconsistent about it in the codebase, but I personally prefer to treat acronyms as words in and of themselves when doing casing, because it makes things a little easier to read (even though control-arrow movement now seems to handle it correctly in Xcode). So I would call this SshKeyItemState
. Again, we're inconsistent and there are good arguments either way, so probably a moot point.
// TODO: PM-10401 create state when SDK is updated | ||
SSHKeyItemState( | ||
isPrivateKeyVisible: false, | ||
privateKey: "TestPrivateKey", |
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.
🤔 Should these be "Test" or just empty?
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 user can't trigger this flow so I'm fine with any content. Maybe "Test" or "TODO" make more sense to explicitly indicate something is needed to be changed there yet.
# Conflicts: # BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings
...rdenShared/UI/Platform/Application/Utilities/Alert/Alert/TestHelpers/Alert+TestHelpers.swift
Outdated
Show resolved
Hide resolved
…TestHelpers/Alert+TestHelpers.swift Co-authored-by: Katherine Bertelsen <kbertelsen@bitwarden.com>
🎟️ Tracking
PM-10403
📔 Objective
Add new SSH key cipher item type view and use it in the cipher item view and edition.
Note: This is not the complete feature as it needs the SSH key item to be added to the SDK to completely work. But added the new behavior in all places possible before the SDK update.
📸 Screenshots
View SSH Key cipher item
Edit SSH Key cipher item
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes