mcgilman commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1537818713
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/ui/common/controller-service/enable-controller-service/enable-controller-service.component.html:
##########
@@ -21,8 +21,8 @@ <h2 mat-dialog-title>Enable Controller Service</h2>
<form class="controller-service-enable-form"
[formGroup]="enableControllerServiceForm">
@if (enableRequest.currentStep === SetEnableStep.Pending) {
<mat-dialog-content>
- <div class="tab-content py-4 flex gap-x-4">
- <div class="w-96 flex flex-col gap-y-4">
+ <div class="tab-content py-4 grid-container grid
grid-cols-2">
+ <div class="col-span-1 pr-5">
Review Comment:
Why the change to `grid`? This applies to both the `enable` and `disable`
service component.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/ui/common/component-state/component-state.component.scss:
##########
@@ -20,8 +20,6 @@
.component-state-dialog {
@include mat.button-density(-1);
- width: 760px;
Review Comment:
Something appears to have caused a number of regressions in our component
state dialog. It doesn't appear to be this PR so I've filed [1] to address
later.
[1] https://issues.apache.org/jira/browse/NIFI-12946
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/tsconfig.json:
##########
@@ -7,7 +7,7 @@
"forceConsistentCasingInFileNames": true,
"strict": true,
"noImplicitOverride": true,
- "noPropertyAccessFromIndexSignature": true,
+ "noPropertyAccessFromIndexSignature": false,
Review Comment:
Why was this changed?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/ui/common/yes-no-dialog/yes-no-dialog.component.html:
##########
@@ -17,7 +17,7 @@
<h2 mat-dialog-title data-qa="yes-no-title">{{ request.title }}</h2>
<mat-dialog-content>
- <div class="text-sm max-w-sm" data-qa="yes-no-message">{{ request.message
}}</div>
Review Comment:
For short messages this works great. However, when the content of this
dialog is lengthy the changes in this PR look too cramped.
<img width="357" alt="Screenshot 2024-03-25 at 12 41 54 PM"
src="https://github.com/apache/nifi/assets/123395/41803e9e-4c89-49fd-bbed-aec4de297d9b";>
Thoughts on identifying instances like these and increasing the width in
those one off cases?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/ui/common/controller-service/enable-controller-service/enable-controller-service.component.html:
##########
@@ -21,8 +21,8 @@ <h2 mat-dialog-title>Enable Controller Service</h2>
<form class="controller-service-enable-form"
[formGroup]="enableControllerServiceForm">
@if (enableRequest.currentStep === SetEnableStep.Pending) {
<mat-dialog-content>
- <div class="tab-content py-4 flex gap-x-4">
- <div class="w-96 flex flex-col gap-y-4">
+ <div class="tab-content py-4 grid-container grid
grid-cols-2">
+ <div class="col-span-1 pr-5">
Review Comment:
The spacing in these dialogs is tighter now because with the change from
`flex` to `grid` the `gap` was lost.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/assets/styles/_listing-table.scss:
##########
@@ -25,7 +25,6 @@
border-style: solid;
table {
- width: 100%;
Review Comment:
This change is leading to horizontal scrolling.
<img width="1595" alt="Screenshot 2024-03-25 at 1 01 26 PM"
src="https://github.com/apache/nifi/assets/123395/e6970a44-babf-4c9e-8f89-3bd24c80c74d";>
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/parameter-contexts/ui/parameter-context-listing/parameter-table/parameter-table.component.html:
##########
@@ -15,8 +15,8 @@
~ limitations under the License.
-->
-<div class="parameter-table flex gap-x-3">
- <div class="flex flex-col gap-y-3">
+<div class="parameter-table grid-container grid grid-cols-3">
+ <div class="col-span-2 pr-5">
Review Comment:
Why change to `grid`? Since we aren't dealing with multiple `rows` I'm not
sure the `grid` is the best option. We should be able to accomplish the same
layout using `flex`.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/access-policies/state/access-policy/access-policy.effects.ts:
##########
@@ -172,7 +173,7 @@ export class AccessPolicyEffects {
concatLatestFrom(() =>
this.store.select(selectAccessPolicy).pipe(isDefinedAndNotNull())),
tap(([, accessPolicy]) => {
const dialogReference =
this.dialog.open(OverridePolicyDialog, {
- panelClass: 'small-dialog'
+ ...DIALOG_SIZES.LARGE
Review Comment:
Was moving from `small` to `large` intentional?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-configuration-history/state/flow-configuration-history-listing/flow-configuration-history-listing.effects.ts:
##########
@@ -95,7 +96,7 @@ export class FlowConfigurationHistoryListingEffects {
ofType(HistoryActions.openPurgeHistoryDialog),
tap(() => {
const dialogReference = this.dialog.open(PurgeHistory, {
- panelClass: 'medium-short-dialog'
+ ...DIALOG_SIZES.LARGE
Review Comment:
I think the `large` dialog may be too big here. Was `medium` not wide enough?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
