Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
mcgilman merged PR #8535: URL: https://github.com/apache/nifi/pull/8535 -- 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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
mcgilman commented on code in PR #8535: URL: https://github.com/apache/nifi/pull/8535#discussion_r1544642143 ## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/ui/canvas/items/processor/edit-processor/relationship-settings/relationship-settings.component.html: ## @@ -15,8 +15,8 @@ ~ limitations under the License. --> - - + + Review Comment: I'm not sure this layout is better. Obviously we need to consider scenarios when the relationship descriptions and lengthy and not. Thoughts? Before: https://github.com/apache/nifi/assets/123395/b0c837ea-d584-4fbd-8eda-f56011203ae9;> After: https://github.com/apache/nifi/assets/123395/4e3adda2-55b8-4c4e-828f-4478e1a77b6c;> -- 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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
mcgilman commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1544632546
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts:
##
@@ -382,9 +383,9 @@ export class ControllerServicesEffects {
const serviceId: string = request.id;
const enableDialogReference =
this.dialog.open(EnableControllerService, {
+...XL_DIALOG,
data: request,
-id: serviceId,
-panelClass: 'large-dialog'
+id: serviceId
Review Comment:
This dialog still doesn't look good. The removal of additional height does
not work. The referencing component tree can be very big. I think we need to
allocate enough (I realize this is subjective) height to accommodate this
listing. The area is designed to scroll for when the listing gets really big.
If we're going to 2/3 and 1/3 I think we probably want to stick with XL since
the referencing component is a tree component which can be nested deeply which
takes up more width. If we want to stick with L maybe 1/2 and 1/2 is more
appropriate.
This comment applies to any place we are showing referencing components.
https://github.com/apache/nifi/assets/123395/4f789f20-9cf6-4a95-8457-84634c330650;>
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
mcgilman commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1544632546
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts:
##
@@ -382,9 +383,9 @@ export class ControllerServicesEffects {
const serviceId: string = request.id;
const enableDialogReference =
this.dialog.open(EnableControllerService, {
+...XL_DIALOG,
data: request,
-id: serviceId,
-panelClass: 'large-dialog'
+id: serviceId
Review Comment:
This dialog still doesn't look good. The removal of additional height does
not work. The referencing component tree can be very big. I think we need to
allocate enough (I realize this is subjective) height so accommodate this
listing. The area is designed so scroll for when the listing gets really big.
If we're going to 2/3 and 1/3 I think we probably want to stick with XL since
the referencing component is a tree component which can be nested deeply. If we
want to stick with L maybe 1/2 and 1/2 is more appropriate.
This comment applies to any place we are showing referencing components.
https://github.com/apache/nifi/assets/123395/4f789f20-9cf6-4a95-8457-84634c330650;>
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1543461479
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts:
##
@@ -382,9 +383,9 @@ export class ControllerServicesEffects {
const serviceId: string = request.id;
const enableDialogReference =
this.dialog.open(EnableControllerService, {
+...XL_DIALOG,
data: request,
-id: serviceId,
-panelClass: 'large-dialog'
+id: serviceId
Review Comment:
ok I gave this another shot using the basis-2/3 and basis-1/3. I think it
works well. There were some other text overflow issues I had to address for
these dialogs but I was able to use the LARGE_DIALOG,
`.basis-2/3`/`.basis-1/3`, text ellipsis, and remove the height to reduce the
amount of unused space.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
rfellows commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1542835501
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts:
##
@@ -382,9 +383,9 @@ export class ControllerServicesEffects {
const serviceId: string = request.id;
const enableDialogReference =
this.dialog.open(EnableControllerService, {
+...XL_DIALOG,
data: request,
-id: serviceId,
-panelClass: 'large-dialog'
+id: serviceId
Review Comment:
Should we be using the `basis-2/3`. and `basis-1/3` here to make the left
side larger than the right? Seems like we probably don't need half of the space
for the referencing components here.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535: URL: https://github.com/apache/nifi/pull/8535#discussion_r1542117048 ## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-configuration-history/ui/flow-configuration-history-listing/purge-history/purge-history.component.html: ## @@ -34,7 +34,7 @@ Purge History - + Review Comment: https://issues.apache.org/jira/browse/NIFI-12964 -- 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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535: URL: https://github.com/apache/nifi/pull/8535#discussion_r1542115674 ## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-configuration-history/ui/flow-configuration-history-listing/purge-history/purge-history.component.html: ## @@ -34,7 +34,7 @@ Purge History - + Review Comment: Yea these widgets are not the same size in the flow configuration history either: https://github.com/apache/nifi/assets/6797571/fb36f7c5-616d-4b39-9aef-727d9f5df590;> -- 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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1542102631
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/flow/flow.effects.ts:
##
@@ -2189,11 +2190,12 @@ export class FlowEffects {
ofType(FlowActions.showOkDialog),
tap((request) => {
this.dialog.open(OkDialog, {
+...MEDIUM_DIALOG,
+maxWidth: '24rem',
Review Comment:
The approach I tried to take in regards to the dialog widths was to keep as
much parody with the current dialog widths as possible but to use the
mat-dialog API to set the dialog sizes and not using the dialog content widths
to determine the dialog width.
One thing to note here: The tailwind css `.max-w-sm` sets the max-width:
24rem; and the `.w-96` sets width: 24rem;.
So when I started looking throughout the nifi application I noticed that the
SMALL_DIALOG size was used almost exclusively for the YesNoDialog use cases. If
you look in the template for the YesNoDialog you will see that I removed the
`.max-w-sm` class from the template. Instead now the SMALL_DIALOG config sets
the maxWidth: '24rem'.
There were three remaining dialogs (other than YesNoDialogs) that used the
SMALL_DIALOG size. The OverridePolicyDialog, CreatePortDialog, and
AddPropertyDialog. For the OverridePolicayDialog the
override-policy-dialog.component.html template set a `.w-96` which as we noted
above sets a width: 24rem;. This is effectively the same thing as using the
`.max-w-sm` class and so we simply can use the SMALL_DIALOG size config for the
OverridePolicyDialog. For the other two SMALL_DIALOG's (CreatePortDialog and
the AddPropertyDialog) the maxWidth: '24rem' works really well so I left them
as SMALL_DIALOG.
The last place we had a width set by a tailwind style in an dialog component
template was in the OkDialog where `.max-w-sm` was setting a max-width: 24rem.
But not all OkDialogs are the same size. Some OkDialog are SMALL_DIALOG and
some OkDialog are MEDIUM_DIALOG. So in order that the OkDialog use cases to
have parody the OkDialogs need to receive their initial size config from a
SMALL_DIALOG or a MEDIUM_DIALOG config but all OkDialogs must also override
their `maxWidth: '24rem'` to account for the removal of the `.max-w-sm` from
the OkDialog template.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1542075055
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts:
##
@@ -382,9 +383,9 @@ export class ControllerServicesEffects {
const serviceId: string = request.id;
const enableDialogReference =
this.dialog.open(EnableControllerService, {
+...XL_DIALOG,
data: request,
-id: serviceId,
-panelClass: 'large-dialog'
+id: serviceId
Review Comment:
Yes this was intentional in order to keep the text for each of the 'Steps to
Disable' on a single row with the green check mark at the end of the single
line.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
mcgilman commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1541475885
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/flow/flow.effects.ts:
##
@@ -2189,11 +2190,12 @@ export class FlowEffects {
ofType(FlowActions.showOkDialog),
tap((request) => {
this.dialog.open(OkDialog, {
+...MEDIUM_DIALOG,
+maxWidth: '24rem',
Review Comment:
I see this is used in a few places. I believe with this set up the
`minWidth` is wider than the `maxWidth`. `SMALL_DIALOG` is already set to use
`24rem`. What was the intention here?
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts:
##
@@ -382,9 +383,9 @@ export class ControllerServicesEffects {
const serviceId: string = request.id;
const enableDialogReference =
this.dialog.open(EnableControllerService, {
+...XL_DIALOG,
data: request,
-id: serviceId,
-panelClass: 'large-dialog'
+id: serviceId
Review Comment:
Was moving to XL dialog intentional? This appears to have been done to both
Enable and Disable dialogs.
https://github.com/apache/nifi/assets/123395/dd52dbfc-b133-4174-8c9a-1fa5c2a4a203;>
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-configuration-history/ui/flow-configuration-history-listing/purge-history/purge-history.component.html:
##
@@ -34,7 +34,7 @@ Purge History
-
+
Review Comment:
I don't _think_ was introduced here but these two fields are different
widths. Happy to file something though and we can fix later unless you want to
get it here.
https://github.com/apache/nifi/assets/123395/312eee5e-b29f-41af-b138-549d630183be;>
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1539869502
##
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 @@
{{ request.title }}
-{{ request.message
}}
Review Comment:
Ok, here is the current approach:
- YesNoDialog's are now all SMALL_DIALOG's now which get the `max-width:
24rem`.
- OkDialogs can be any size dialog but should also have `maxWidth: '24rem'`
override when opening the dialog
- Cancel dialog messages are controlled by the frontend and are small. No
dialog size or max width are set on these
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1538335965
##
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 @@ Enable Controller Service
@if (enableRequest.currentStep === SetEnableStep.Pending) {
-
-
+
+
Review Comment:
I made this dialog a bit wider so I was playing around with the grid layout
to split the width in half. I will restore this spacing.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1538328261
##
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:
We could also remove any dialog size config for this dialog and allow it to
just be as wide as it needs to be.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1538325190
##
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 @@
{{ request.title }}
-{{ request.message
}}
Review Comment:
So I think the YesNoDialog, CancelDialog, and OkDialog can all not be
designated as SMALL dialogs and this will help this use case.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1538319881
##
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:
ok I can restore this.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1538306100
##
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 @@
{{ request.title }}
-{{ request.message
}}
Review Comment:
Yes, I think I have an idea. We can remove the SMALL dialog size designation
from these types of dialogs. Doing so for Empty All Queues Dialog would look
like:
https://github.com/apache/nifi/assets/6797571/d505de0c-bea0-4021-99ff-44b4f187c32a;>
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535: URL: https://github.com/apache/nifi/pull/8535#discussion_r1538282376 ## 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. --> - - + + Review Comment: I could have used flex for this element but then I needed to assign a width to at least one of (and likely it should be both) the listing table and the referencing components elements. Instead I opted to use the grid to give 2/3 width to the parameter listing table and leave 1/3 of the space to the referencing components container element. I think either approach is appropriate here but let me know if you think we need to make a change here. Using flex as previous: https://github.com/apache/nifi/assets/6797571/d2d6043f-8f21-4843-bb15-ef644dbc20cd;> -- 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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535: URL: https://github.com/apache/nifi/pull/8535#discussion_r1538282376 ## 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. --> - - + + Review Comment: I could have used flex for this element but then I needed to assign a width to at least one of (and likely it should be both) the listing table and the referencing components elements. Instead I opted to use the grid to give 2/3 width to the parameter listing table and leave 1/3 of the space to the referencing components container element. I think either approach is appropriate here but let me know if you think we need to make a change here. Using flex as previous: ![Uploading Screenshot 2024-03-25 at 5.56.32 PM.png…]() -- 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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535: URL: https://github.com/apache/nifi/pull/8535#discussion_r1538282376 ## 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. --> - - + + Review Comment: I could have used flex for this element but then I needed to assign a width to at least one of (and likely it should be both) the listing table and the referencing components elements. Instead I opted to use the grid to give 2/3 width to the parameter listing table and leave 1/3 of the space to the referencing components container element. I think either approach is appropriate here but let me know if you think we need to make a change here. -- 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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1538278902
##
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:
It looks like the `.listing-table` class is on the wrong DOM element. I can
update it here.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535: URL: https://github.com/apache/nifi/pull/8535#discussion_r1538271623 ## 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: I was getting an error when trying to access `DIALOG_SIZES.LARGE` or `DIALOG_SIZES.MEDIUM`. I think we want to keep this config. -- 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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1538270150
##
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:
It was just a personal preference. I thought having the instructions on a
single line was easier to comprehend and it made it more clear to the user that
they need to select either the "Copy" or the "Empty" policy will be the default:
Previously:
https://github.com/apache/nifi/assets/6797571/a535ad0b-9b3c-4ac3-b7e8-7a59d0d4;>
If we make this a large dialog maybe it is more clear to the user what to do:
https://github.com/apache/nifi/assets/6797571/24a2c066-0db8-485f-a03d-ea193e7dec40;>
Please let me know your thoughts here. I can easily restore the small dialog
if you think it is appropriate.
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
scottyaslan commented on code in PR #8535:
URL: https://github.com/apache/nifi/pull/8535#discussion_r1538266243
##
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:
This one was a bit special as it was the only 'medium-short-dialog'. I tried
to go with the MEDIUM dialog here but Angular Material puts a `width: 180px` on
the `.mat-mdc-form-field-infix` class. In a MEDIUM dialog like this where these
two input fields are next to each other in the UX then the input field takes up
236px. Two inputs plus padding in between and on either side would be too wide
to fit into a 460px wide MEDIUM dialog:
https://github.com/apache/nifi/assets/6797571/4251e3a7-63c1-4b01-b733-a5d8046ec683;>
--
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
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
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 @@ Enable Controller Service
@if (enableRequest.currentStep === SetEnableStep.Pending) {
-
-
+
+
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 @@
{{ request.title }}
-{{ request.message
}}
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.
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 @@ Enable Controller Service
@if (enableRequest.currentStep === SetEnableStep.Pending) {
-
-
+
+
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.
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.
-->
-
-
+
+
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'
+
Re: [PR] [NIFI-12922] semantic dialog size configurations [nifi]
mcgilman commented on PR #8535: URL: https://github.com/apache/nifi/pull/8535#issuecomment-2007727537 Will review... -- 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
