Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[NIFI-13207] page headers and refresh containers are consistently pos… #8804

Merged
merged 13 commits into from
May 23, 2024
Merged

[NIFI-13207] page headers and refresh containers are consistently pos… #8804

merged 13 commits into from
May 23, 2024

Conversation

scottyaslan
Copy link
Contributor

…itioned between pages
Kapture 2024-05-09 at 20 45 50

@scottyaslan scottyaslan added the new ui Pull requests for work relating to the new user interface being developed. label May 10, 2024
@mcgilman
Copy link
Contributor

Reviewing...

rfellows
rfellows approved these changes May 10, 2024
View reviewed changes
Copy link
Contributor

@rfellows rfellows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

mcgilman
mcgilman reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@mcgilman mcgilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @scottyaslan! Left a few notes below.

...rc/app/pages/access-policies/ui/global-access-policies/global-access-policies.component.html Show resolved Hide resolved
...nce/ui/provenance-event-listing/provenance-event-table/provenance-event-table.component.html Show resolved Hide resolved
...-frontend/src/main/nifi/src/app/pages/access-policies/feature/access-policies.component.html Outdated Show resolved Hide resolved
@scottyaslan scottyaslan force-pushed the NIFI-13207 branch 2 times, most recently from d24c7f1 to fbbdf83 Compare May 15, 2024 01:25
@scottyaslan
Copy link
Contributor Author

Update:

All page headers should have 1.25rem padding from the navigation toolbar at the top of each page and they should have 1.25rem padding below:

Screenshot 2024-05-14 at 9 41 59 PM

Any text above the filter form fields should have 0.75rem padding below:

Screenshot 2024-05-14 at 9 43 01 PM

Any hint text should have 0.25rem padding above and 0.5rem padding below:

Screenshot 2024-05-14 at 9 42 27 PM Screenshot 2024-05-14 at 9 42 37 PM

For the footer (last updated etc) it should have 0.5rem padding above and 1.25rem padding below:

Screenshot 2024-05-14 at 9 43 44 PM Screenshot 2024-05-14 at 9 43 57 PM

Tabs (not in dialogs) should have 1.25rem padding below:

Screenshot 2024-05-14 at 9 50 45 PM

@scottyaslan
Copy link
Contributor Author

@mcgilman a good way to quickly see these changes is to use the browser back/forward navigation between the pages. The changes at the top of the page are particularly noticeable in that they are more consistent with the spacing:

Kapture 2024-05-15 at 09 58 28

A more tedious check would be to go through each page and verify all the padding/spacing and see that there is no longer overlapping elements nor any need for negative margins etc. Feel free to use both approaches.

mcgilman
mcgilman reviewed May 15, 2024
View reviewed changes
Copy link
Contributor

@mcgilman mcgilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @scottyaslan! I've noted a few items below.

.../pages/access-policies/ui/component-access-policies/component-access-policies.component.html Outdated Show resolved Hide resolved
...-web-frontend/src/main/nifi/src/app/ui/common/component-state/component-state.component.html Outdated Show resolved Hide resolved
...b/nifi-web-frontend/src/main/nifi/src/app/ui/common/error-banner/error-banner.component.html Outdated Show resolved Hide resolved
.../nifi/src/app/pages/cluster/ui/common/cluster-table-filter/cluster-table-filter.component.ts Outdated Show resolved Hide resolved
@scottyaslan scottyaslan requested a review from mcgilman May 16, 2024 15:31
mcgilman
mcgilman reviewed May 17, 2024
View reviewed changes
Copy link
Contributor

@mcgilman mcgilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @scottyaslan! Looks good... just noted a few minor things below.

.../pages/access-policies/ui/component-access-policies/component-access-policies.component.html Outdated Show resolved Hide resolved
...app/pages/bulletins/ui/bulletin-board/bulletin-board-list/bulletin-board-list.component.html Outdated Show resolved Hide resolved
...tory/ui/flow-configuration-history-listing/flow-configuration-history-listing.component.html Outdated Show resolved Hide resolved
...i-web/nifi-web-frontend/src/main/nifi/src/app/pages/settings/feature/settings.component.html
@@ -35,7 +35,7 @@ <h3 class="primary-color">NiFi Settings</h3>
}
</nav>
</div>
<div class="pt-4 flex-1">
<div class="pt-5 flex-1">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok for the General tab but appears to be too much padding for tabs that include tables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually consistent for all pages with tabs. Each of them receive 1.25rem of padding before the first form element is displayed. For cases like the General tab here or the Summary tables this padding seems comfortable:

Screenshot 2024-05-20 at 5 50 22 PM Screenshot 2024-05-20 at 6 01 27 PM

But for the Management Controller Services, Reporting Tasks, Flow Analysis Rules, Registry Clients, and Parameter Providers tabs at appears there is more space:

Screenshot 2024-05-20 at 5 50 04 PM

but really this is an optical illusion. If we look more closely we can see that the "Create" mat-icon button on the top right of the tables in each of these views is directly beneath the padding of the tabs:

Screenshot 2024-05-20 at 5 54 46 PM Screenshot 2024-05-20 at 5 49 48 PM

I am happy to discuss options here...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did correct some missing padding in the summary table filter component to account for the input field placeholder labels. Now they all get some .pt-2 to account for the placeholder text:

Screenshot 2024-05-20 at 9 53 10 PM Screenshot 2024-05-20 at 9 53 34 PM Screenshot 2024-05-20 at 9 53 54 PM

...ifi/src/app/pages/cluster/ui/common/cluster-table-filter/cluster-table-filter.component.html Outdated Show resolved Hide resolved
@scottyaslan scottyaslan requested a review from mcgilman May 20, 2024 22:57
@scottyaslan scottyaslan force-pushed the NIFI-13207 branch 2 times, most recently from 60920e6 to 45e5963 Compare May 21, 2024 18:04
mcgilman
mcgilman approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@mcgilman mcgilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @scottyaslan!

@mcgilman mcgilman merged commit 62d5502 into apache:main May 23, 2024
5 checks passed
shubhluck pushed a commit to shubhluck/nifi that referenced this pull request Jun 1, 2024
@scottyaslan @shubhluck
[NIFI-13207] page headers and refresh containers are consistently pos… (
b5b1809 
apache#8804)

* [NIFI-13207] page headers and refresh containers are consistently positioned between pages

* pad error banner

* display hint below form fields to follow material spec, updating padding/spacing accordingly

* revert access policies template

* remove unused MatHint

* access policy status as hint and center align hints

* use margin bottom on error banner and more spacing improvements

* update hint spacing in a few more use cases

* remove extra padding from bottom of nifi cluster table filter component

* collapse hint height when no text to display

* update padding for input form field placeholder padding

* use margins instead of padding

* final touches

This closes apache#8804
shubhluck pushed a commit to shubhluck/nifi that referenced this pull request Jun 1, 2024
@scottyaslan @shubhluck
[NIFI-13207] page headers and refresh containers are consistently pos… (
05132d1 
apache#8804)

* [NIFI-13207] page headers and refresh containers are consistently positioned between pages

* pad error banner

* display hint below form fields to follow material spec, updating padding/spacing accordingly

* revert access policies template

* remove unused MatHint

* access policy status as hint and center align hints

* use margin bottom on error banner and more spacing improvements

* update hint spacing in a few more use cases

* remove extra padding from bottom of nifi cluster table filter component

* collapse hint height when no text to display

* update padding for input form field placeholder padding

* use margins instead of padding

* final touches

This closes apache#8804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcgilman mcgilman mcgilman approved these changes

@rfellows rfellows Awaiting requested review from rfellows

Assignees
No one assigned
Labels
new ui Pull requests for work relating to the new user interface being developed.
Projects
None yet
Milestone
No milestone
3 participants
@scottyaslan @mcgilman @rfellows