-
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
[CL-508] extension width setting #12040
Conversation
New Issues
Fixed Issues
|
Whoops, let me fix those broken tests. edit: fixed in b8ee6a0 |
3b6b767
to
b8ee6a0
Compare
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12040 +/- ##
==========================================
- Coverage 33.48% 33.47% -0.01%
==========================================
Files 2851 2876 +25
Lines 89283 89871 +588
Branches 17008 17104 +96
==========================================
+ Hits 29900 30088 +188
- Misses 57029 57407 +378
- Partials 2354 2376 +22 ☔ View full report in Codecov by Sentry. |
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.
Do we want to have some stories of the popup layout at different widths?
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.
Feedback from DS team call:
- Lets update the field label to "Extension width"
- Lets update the behavior so when the extension is popped out the popout's width defaults to the same as the "Extension width" setting.
fe47454
to
88d536f
Compare
88d536f
to
5573a86
Compare
|
type PopupWidthOptions = typeof PopupWidthOptions; | ||
export type PopupWidthOption = keyof PopupWidthOptions; | ||
|
||
const POPUP_WIDTH_KEY_DEF = new KeyDefinition<PopupWidthOption>(THEMING_DISK, "popup-width", { |
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.
@justindbaur When is it appropriate to share state definitions? Width is semantically related to theming, and it feels like we aren't meant to create a new state definition for every little piece of state, right? (Otherwise, why have key definitions at all?)
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.
If you have similar things you can share them but one of the important rules is you shouldn't share them between teams and most likely shouldn't share them to "far" away from each other. Because the problem you could run into is someone else makes a key with the same name within a state definition that someone else does. So I think since this would be in DS ownership it should use its own state definition.
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.
Boom: 0fc38b9 (thanks)
0fc38b9
libs/common/src/platform/services/config/default-config.service.ts
Outdated
Show resolved
Hide resolved
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.
Just one optional comment but otherwise looks good.
6f1f799
🎟️ Tracking
https://bitwarden.atlassian.net/browse/CL-508
📔 Objective
Adds an setting to the extension for configuring the popup's width. 🚀
📸 Screenshots
Screen.Recording.2024-11-18.at.2.02.56.PM.mov
⏰ 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