-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-11899] - send text details component #11002
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11002 +/- ##
=======================================
Coverage ? 34.96%
=======================================
Files ? 2701
Lines ? 84282
Branches ? 16016
=======================================
Hits ? 29472
Misses ? 53841
Partials ? 969 ☔ View full report in Codecov by Sentry. |
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.
Only a partial review
As discussed I'm uncertain about the changes to the send and sendApi services, which we should likely revert and focus this PR on the UI changes. If it breaks saving Sends on the feature-flagged part, we should fix it in a separate PR.
@@ -23,6 +23,7 @@ export function extensionRefreshSwap( | |||
defaultComponent, | |||
refreshedComponent, | |||
async () => { | |||
return true; |
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.
@@ -26,6 +26,7 @@ export class SendView implements View { | |||
password: string = null; | |||
disabled = false; | |||
hideEmail = false; | |||
hideText = false; |
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.
Sounds good. The type changes were only a narrowing of types so reverting back to |
"textHiddenByDefault": { | ||
"message": "When accessing the Send, hide the text by default", | ||
"description": "'Send' is a noun and the name of a feature called 'Bitwarden Send'. It should not be translated." | ||
}, |
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.
hideTextByDefault
which is used.
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.
Addressed with efffcc6
"limitSendViews": { | ||
"message": "Limit views" | ||
}, | ||
"limitSendViewsHint": { | ||
"message": "No one can view this Send after the limit is reached.", | ||
"description": "Displayed under the limit views field on Send" | ||
}, | ||
"limitSendViewsHintWithCount": { | ||
"message": "No one can view this Send after the limit is reached. $ACCESSCOUNT$ views left", | ||
"description": "Displayed under the limit views field on Send", | ||
"placeholders": { | ||
"accessCount": { | ||
"content": "$1", | ||
"example": "2" | ||
} | ||
} | ||
}, |
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.
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.
Addressed with efffcc6
<bit-form-field> | ||
<bit-label>{{ "sendTypeTextToShare" | i18n }}</bit-label> | ||
<textarea bitInput id="text" rows="6" formControlName="text"></textarea> | ||
<bit-hint>{{ "sendTextDesc" | i18n }}</bit-hint> |
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.
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.
Addressed with 6c7ad7c
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.
@jaasen-livefront Addressed some minor issues myself and this is now ready to merge.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-11899
📔 Objective
This PR adds the send text details component with a corresponding send details and base send component to allow shared use of the logic and structure for the upcoming send file details component. This is one of three PRs for the new create send UI as part of the extension refresh. Credit goes to @djsmith85 for helping set this up.
📸 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