rfellows commented on code in PR #8608:
URL: https://github.com/apache/nifi/pull/8608#discussion_r1556093084
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/access-policies/ui/common/add-tenant-to-policy-dialog/add-tenant-to-policy-dialog.component.html:
##########
@@ -36,14 +36,15 @@ <h2 mat-dialog-title>Add Users/Groups To Policy</h2>
<mat-selection-list formControlName="userGroups"
class="border">
@for (userGroup of filteredUserGroups; track userGroup) {
<mat-list-option togglePosition="before"
[value]="userGroup.id">
- <i class="fa fa-users mr-3"></i>{{
userGroup.component.identity }}
+ <i class="fa fa-users
mat-primary-on-default-surface mr-3"></i
Review Comment:
Should this use `mat-primary` instead?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/service/manager/process-group-manager.service.ts:
##########
@@ -735,7 +735,7 @@ export class ProcessGroupManager {
// update transmitting
const transmitting = details
.select('text.process-group-transmitting')
- .classed('transmitting', function (d: any) {
+ .classed('nifi-success-default', function (d: any) {
Review Comment:
Same question about the transmitting color here. Should it be the success
color? It used to be blue.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/summary/ui/process-group-status-listing/process-group-status-table/process-group-status-table.component.ts:
##########
@@ -98,23 +98,23 @@ export class ProcessGroupStatusTable extends
ComponentStatusTable<ProcessGroupSt
private versionedFlowStateMap: { [key: string]: { classes: string; label:
string } } = {
STALE: {
- classes: 'fa fa-arrow-circle-up stale',
+ classes: 'fa fa-arrow-circle-up nifi-warn-light',
Review Comment:
stale looks good on this background with the nifi-warn-light color. i think
it is just the case on the canvas when it is on the light blue background that
this color looks a bit washed out.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/summary/ui/common/cluster-summary-dialog/remote-process-group-cluster-table/remote-process-group-cluster-table.component.ts:
##########
@@ -112,7 +112,7 @@ export class RemoteProcessGroupClusterTable extends
ComponentClusterTable<NodeRe
getTransmissionStatusIcon(rpg: NodeRemoteProcessGroupStatusSnapshot):
string {
if (rpg.statusSnapshot.transmissionStatus === 'Transmitting') {
- return 'transmitting fa fa-bullseye';
+ return 'nifi-success-default fa fa-bullseye';
Review Comment:
just flagging the transmitting change again in case we want to update it
back to using the blue color.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/service/manager/process-group-manager.service.ts:
##########
@@ -1079,15 +1079,16 @@ export class ProcessGroupManager {
if (self.isUnderVersionControl(processGroupData)) {
const vciState =
processGroupData.versionedFlowState;
if (vciState === 'SYNC_FAILURE') {
- return `version-control primary-contrast-A700`;
+ return `version-control nifi-surface-default`;
} else if (vciState ===
'LOCALLY_MODIFIED_AND_STALE') {
- return `version-control warn-400`;
+ return `version-control nifi-warn-lighter`;
} else if (vciState === 'STALE') {
- return `version-control warn-400`;
+ return `version-control nifi-warn-lighter`;
} else if (vciState === 'LOCALLY_MODIFIED') {
- return `version-control primary-contrast-A700`;
+ return `version-control nifi-surface-default`;
Review Comment:
This doesn't seem to have enough contrast in dark mode...
<img width="411" alt="Screenshot 2024-04-08 at 12 38 15 PM"
src="https://github.com/apache/nifi/assets/713866/6a1133b1-3a6e-4d06-889b-4f36a8cf470e";>
<img width="409" alt="Screenshot 2024-04-08 at 12 38 25 PM"
src="https://github.com/apache/nifi/assets/713866/1b0ac59d-9205-4706-aa81-2e1ff6a57f99";>
Might not need to fix it here, just caught my eye.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/ui/manage-remote-ports/manage-remote-ports.component.html:
##########
@@ -197,7 +197,7 @@ <h3 class="text-xl bold manage-remote-ports-header
pb-5">Manage Remote Ports</h3
@if (currentRpg) {
@if (port.transmitting) {
<div
- class="pointer
transmitting fa fa-bullseye"
+ class="pointer
nifi-success-default fa fa-bullseye"
Review Comment:
same question about transmitting.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/service/manager/port-manager.service.ts:
##########
@@ -449,7 +449,7 @@ export class PortManager {
return '\ue80a';
}
})
- .classed('transmitting', function (d: any) {
+ .classed('nifi-success-default', function (d: any) {
Review Comment:
Is this the right color? the transmitting color before was a bluish color,
now it is green.
```
A400: #44a3cf, // .enabled, .transmitting, .load-balance-icon-active
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/summary/ui/remote-process-group-status-listing/remote-process-group-status-table/remote-process-group-status-table.component.ts:
##########
@@ -109,7 +109,7 @@ export class RemoteProcessGroupStatusTable extends
ComponentStatusTable<RemotePr
getTransmissionStatusIcon(rpg: RemoteProcessGroupStatusSnapshotEntity):
string {
if (rpg.remoteProcessGroupStatusSnapshot.transmissionStatus ===
'Transmitting') {
- return 'transmitting fa fa-bullseye';
+ return 'nifi-success-default fa fa-bullseye';
Review Comment:
another transmitting color change
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/service/manager/remote-process-group-manager.service.ts:
##########
@@ -613,7 +613,7 @@ export class RemoteProcessGroupManager {
.classed('invalid', function (d: any) {
return self.hasIssues(d);
})
- .classed('transmitting', function (d: any) {
+ .classed('nifi-success-default', function (d: any) {
Review Comment:
should transmitting really be the success color?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/service/manager/process-group-manager.service.ts:
##########
@@ -791,7 +791,7 @@ export class ProcessGroupManager {
// update running
const running = details
.select('text.process-group-running')
- .classed('running', function (d: any) {
+ .classed('nifi-success-lighter', function (d: any) {
Review Comment:
What are your thoughts on leaving the named classes as purely a way to
indicate the state on the dom element? We don't need to define any styles for
these classes, but they would just be markers to help us identify what the
state is supposed to be. We would still use the new color classes to apply the
appropriate color though.
Just in reviewing this PR i find it difficult to determine what the state is
since we re-use some colors for more than one state.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
