Skip to content

[PM-42462] fix: Show Select as the vault item picker placeholder - #2992

Open
andrebispo5 wants to merge 2 commits into
mainfrom
pm-42462-brand-name-select-placeholder
Open

[PM-42462] fix: Show Select as the vault item picker placeholder#2992
andrebispo5 wants to merge 2 commits into
mainfrom
pm-42462-brand-name-select-placeholder

Conversation

@andrebispo5

@andrebispo5 andrebispo5 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-42462

📔 Objective

  • The card brand picker showed --Select-- instead of Select like the designs ask for.
  • Card expiration month, identity title, and bank account type used the same format, so I fixed all four. Fixing only the brand would leave two placeholder styles on the same screen.
  • Tidied up the doc comments on those four so they match. A couple said "default state title for title type" even on the card brand.
  • Added a test per type so the dashes don't come back.

📸 Screenshots

Before After

@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug labels Aug 25, 2026
@andrebispo5
andrebispo5 marked this pull request as ready for review August 25, 2026 17:19
@andrebispo5
andrebispo5 requested review from a team and matt-livefront as code owners August 25, 2026 17:19
Copilot AI lite review requested due to automatic review settings August 25, 2026 17:19
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This change replaces the --Select-- placeholder with the localized Select string for the four vault item pickers (card brand, card expiration month, identity title, bank account type) and aligns their doc comments. I verified no other --Select-- style placeholders remain in the codebase, that Localizations.select is present across the localization catalogs, and that nothing compares against the previous placeholder string. Each of the four types gained a matching test; the new TitleTypeTests.swift follows the existing Swift Testing conventions used by BankAccountTypeTests.

Code Review Details

No findings.

Notes considered and dismissed: the four defaultValueLocalizedName implementations remain duplicated, but that duplication predates this PR; and the disabled snapshot baselines still show the old text, which is expected while snapshot tests are disabled.

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.82%. Comparing base (dede418) to head (05dd87e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2992      +/-   ##
==========================================
+ Coverage   79.52%   81.82%   +2.29%     
==========================================
  Files        1169     1046     -123     
  Lines       75095    67798    -7297     
==========================================
- Hits        59723    55474    -4249     
+ Misses      15372    12324    -3048     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates several Menuable.defaultValueLocalizedName implementations to match design expectations by showing the localized Select placeholder without surrounding dashes, and adds regression tests to prevent reintroducing the --Select-- format.

Changes:

  • Replace "--\(Localizations.select)--" with Localizations.select for card brand, card expiration month, identity title, and bank account type placeholders.
  • Align/clarify doc comments for the affected defaultValueLocalizedName properties.
  • Add/extend unit tests to assert the placeholder is Localizations.select for each affected type.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditCardItem/CardComponent.swift Updates card brand + expiration month placeholders to use Localizations.select directly and adjusts doc comments.
BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditCardItem/CardComponentTests.swift Adds XCTest coverage asserting the default placeholder for brand and month is Localizations.select.
BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditBankAccountItem/AddEditBankAccountItemState.swift Updates bank account type placeholder to Localizations.select.
BitwardenShared/Core/Vault/Models/Enum/TitleType.swift Updates title placeholder to Localizations.select and clarifies its doc comment.
BitwardenShared/Core/Vault/Models/Enum/TitleTypeTests.swift Adds Swift Testing coverage asserting the default placeholder is Localizations.select.
BitwardenShared/Core/Vault/Models/Enum/BankAccountTypeTests.swift Adds Swift Testing coverage asserting the default placeholder is Localizations.select.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@fedemkr fedemkr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changes look good, although I left a comment on the Figma design as I'm not sure if leaving plain "Select" here is the best option as it could be a bit confusing IMO.

@andrebispo5

Copy link
Copy Markdown
Contributor Author

Changes look good, although I left a comment on the Figma design as I'm not sure if leaving plain "Select" here is the best option as it could be a bit confusing IMO.

Totally agree on this one, having the Select option there makes it a bit confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants