[PM-12732] - fix new send button #11266
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #11266 +/- ##
==========================================
+ Coverage 33.20% 33.22% +0.02%
==========================================
Files 2727 2730 +3
Lines 85175 85257 +82
Branches 16204 16221 +17
==========================================
+ Hits 28279 28326 +47
- Misses 54643 54675 +32
- Partials 2253 2256 +3 ☔ View full report in Codecov by Sentry. |
|
Fixed Issues
|
| get iconClass() { | ||
| return this.showIcon ? "bwi bwi-plus-f" : null; | ||
| } | ||
|
|
There was a problem hiding this comment.
❓ Why have this getter if the *ngIf within the mark up can already hide show the icon?
| @@ -1,7 +1,7 @@ | |||
| <popup-page> | |||
| <popup-header slot="header" [pageTitle]="'send' | i18n"> | |||
| <ng-container slot="end"> | |||
| <tools-new-send-dropdown *ngIf="!sendsDisabled"></tools-new-send-dropdown> | |||
| <tools-new-send-dropdown [showIcon]="true" *ngIf="!sendsDisabled"></tools-new-send-dropdown> | |||
There was a problem hiding this comment.
I think I'd prefer an optional hideIcon as the more common usage is the + New instead of New Send. Considering the future usage within web and potentially desktop
Which would mean instead of changing line 4, you'd need to modify line 26
| @@ -1,6 +1,6 @@ | |||
| <button bitButton [bitMenuTriggerFor]="itemOptions" buttonType="primary" type="button"> | |||
| <i class="bwi bwi-plus-f" aria-hidden="true"></i> | |||
| {{ "new" | i18n }} | |||
| <i *ngIf="iconClass" class="{{ iconClass }}" aria-hidden="true"></i> | |||
There was a problem hiding this comment.
The class being filled by the getter iconClass is unnecessary as it can always be bwi bwi-plus-f
| <i class="bwi bwi-plus-f" aria-hidden="true"></i> | ||
| {{ "new" | i18n }} | ||
| <i *ngIf="iconClass" class="{{ iconClass }}" aria-hidden="true"></i> | ||
| {{ (iconClass ? "new" : "createSend") | i18n }} |
There was a problem hiding this comment.
This can then also be achieved via the hideIcon input. Modifying the text, using the same input/getter for the icon is not ideal and my suggestion doesn't improve that. I'm open to ideas, but also fine with keeping it as suggested
|
@djsmith85 Agree 💯% with your remarks. This was overly complicated for such a simple implementation. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12732
📔 Objective
This PR fixes the create send button shown in the main window when no sends are active.
📸 Screenshots
⏰ 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