[PM-13818] Allow user to edit self-hosted url during registration #11790
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #11790 +/- ##
==========================================
- Coverage 33.55% 33.55% -0.01%
==========================================
Files 2814 2814
Lines 87435 87440 +5
Branches 16674 16676 +2
==========================================
Hits 29337 29337
- Misses 55793 55798 +5
Partials 2305 2305 ☔ View full report in Codecov by Sentry. |
…-url-during-registration
| private handleSelfHostedSelection(prevSelectedRegion: RegionConfig | Region.SelfHosted | null) { | ||
| return from(SelfHostedEnvConfigDialogComponent.open(this.dialogService)).pipe( | ||
| tap((result: boolean | undefined) => | ||
| this.handleSelfHostedEnvConfigDialogResult(result, prevSelectedRegion), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Converting the open promise to an observable requires a lot of ceremony that isn't buying us anything, since we are effectively converting it back to a promise with take(1) and subscribe() in onSelectClosed.
What do you think of this alternative?
type RegionSelection = RegionConfig | Region.SelfHosted | null;
protected async onSelectClosed(selection: RegionSelection) {
if (selection === Region.SelfHosted) {
const result = await SelfHostedEnvConfigDialogComponent.open(this.dialogService)
return this.handleSelfHostedEnvConfigDialogResult(result, selection);
}
}|
|
||
| protected selectedValue: T; | ||
| protected selectedOption: Option<T>; | ||
| protected searchInputId = `bit-select-search-input-${nextId++}`; | ||
| protected lastSelectedValue: T; |
There was a problem hiding this comment.
How does this differ from selectedValue?
There was a problem hiding this comment.
This is tricky because the selectedValue doesn't update when we select the option twice. registration-env-selector.component.ts doesn't pick up the changes(this.selectedRegion.valueChanges). When onClose is called this.selectedValue doesn't seem to reflect the currently selected value, but the previously selected value (maybe some race condition or async call somewhere?). Any ideas?
I updated the variable name to userSelectedValue which is maybe a little clearer on the intent?
| if (this.lastSelectedValue !== undefined) { | ||
| this.closed.emit(this.lastSelectedValue); | ||
| } |
There was a problem hiding this comment.
I feel like we should always emit, since the event is called "closed" and the select is indeed closing.
…-url-during-registration
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-13818?atlOrigin=eyJpIjoiZmQ2Njc1OTg0NTg5NGI1NDkxMjdhZmQ0NThiOGJmZjciLCJwIjoiaiJ9
📔 Objective
Re-launch the
self-hosted-env-config-dialogafter a user has previously selected the self hosted option from the "Creating account on" select. This allows a user to go back and edit values after the dialog has closed.📸 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