-
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-13808 - Use new <bit-form-field> component in Excluded domains settings. #13111
base: main
Are you sure you want to change the base?
Conversation
Changing to new <bit-form-field> component necessitates modifying form to use ReactiveForms conventions. To handle this change, the deletable/uneditable domain state and new form control state are managed separately, including clearing the form state on save. Fields which compute vaules from the former state should now compute both values (see domain count); [formGroup], formArrayName, formControlName all necessary directives/properties on form and children.
New Issues (1)Checkmarx found the following issues in this Pull Request
Fixed Issues (1)Great job! The following issues were fixed in this Pull Request
|
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.
screenshot looks good; approved by design
@danielleflinn to clarify—that screenshot came from the original ticket verbatim—here's an up-to-date screenshot. ![]() |
@blackwood thank you! Typically the "screenshots" section in PRs is reserved for what the PR changes; you don't need to include screenshots from jira. Sometimes devs will include before/afters. Is there a way for you to remove the bottom margin from the "website" inputs? If you are using a component library field I think there is a disableMargin option. |
2bb8821
to
76271b1
Compare
…int, and type checks).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13111 +/- ##
==========================================
+ Coverage 35.30% 35.37% +0.07%
==========================================
Files 2996 3010 +14
Lines 90959 91180 +221
Branches 16980 16997 +17
==========================================
+ Hits 32116 32258 +142
- Misses 56351 56416 +65
- Partials 2492 2506 +14 ☔ View full report in Codecov by Sentry. |
@@ -14,19 +14,36 @@ | |||
<bit-section *ngIf="!isLoading"> | |||
<bit-section-header> | |||
<h2 bitTypography="h6">{{ "domainsTitle" | i18n }}</h2> | |||
<span bitTypography="body2" slot="end">{{ excludedDomainsState?.length || 0 }}</span> | |||
<span bitTypography="body2" slot="end">{{ | |||
excludedDomainsState?.length + domainForms?.value?.length |
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 the use of optional chaining here is correctly indicating either of these values can be undefined, we should guard against that
… for form controls existing outside of stored values.
Changing to new component necessitates modifying form to use ReactiveForms conventions. To handle this change, the deletable/uneditable domain state and new form control state are managed separately, including clearing the form state on save. Fields which compute vaules from the former state should now compute both values (see domain count); [formGroup], formArrayName, formControlName all necessary directives/properties on form and children.
🎟️ Tracking
Excluded Domain field is incorrect component/styling - Jira
📔 Objective
See image below
📸 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