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.

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

Move Node monitoring option to app bar #8381

Merged
merged 9 commits into from
Sep 9, 2023

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Aug 16, 2023

PR to move the 'Node monitoring' option to the Nodes' app bar from the side panel. Also slight tweaks to the page to remove the sidebar and adds a short description to explain what the pages does.

Before

image

After

image

Before

image

After

image

Testing done

  • Node monitoring works as before

Proposed changelog entries

  • Move Node monitoring option to app bar.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@jenkinsci/sig-ux

Before the changes are marked as ready-for-merge:

Maintainer checklist

@timja timja added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Aug 16, 2023
@timja timja requested a review from a team August 16, 2023 10:55
@timja timja added the needs-security-review Awaiting review by a security team member label Aug 16, 2023
@timja timja requested a review from a team August 16, 2023 10:55
@daniel-beck
Copy link
Member

What underlying principle motivates this change? It does not seem to be justified by https://weekly.ci.jenkins.io/design-library/AppBar/

@janfaracik
Copy link
Contributor Author

What underlying principle motivates this change? It does not seem to be justified by https://weekly.ci.jenkins.io/design-library/AppBar/

It'd align more so with https://weekly.ci.jenkins.io/design-library/Layouts/ 'Don't include actions or external pages in your sidebar, these belong in the top app bar'. It'd be consistent with the 'Log Recorders' interface.

Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

Seems to be fine security wise

@Wadeck Wadeck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Aug 18, 2023
@timja
Copy link
Member

timja commented Aug 18, 2023

Test failure needs looking at @janfaracik

@timja timja added the ath-successful This PR has successfully passed the full acceptance-test-harness suite label Aug 20, 2023
@timja timja requested a review from NotMyFault August 20, 2023 15:10
@timja
Copy link
Member

timja commented Aug 21, 2023

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 21, 2023
@daniel-beck
Copy link
Member

'Don't include actions or external pages in your sidebar, these belong in the top app bar'. It'd be consistent with the 'Log Recorders' interface.

This rule basically invalidates every RootAction.

@basil
Copy link
Member

basil commented Aug 24, 2023

This change broke some core tests due to a markup change, so how do we know that it won't break plugin tests if we haven't run PCT?

@daniel-beck daniel-beck removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 24, 2023
@NotMyFault
Copy link
Member

This change broke some core tests due to a markup change, so how do we know that it won't break plugin tests if we haven't run PCT?

=> jenkinsci/bom#2432

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Great!

@NotMyFault NotMyFault added the pct-successful This PR has successfully passed the full plugin-compatibility-test suite label Aug 29, 2023
@NotMyFault
Copy link
Member

'
/label ready-for-merge


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 5, 2023
@MarkEWaite MarkEWaite merged commit 30540d7 into jenkinsci:master Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ath-successful This PR has successfully passed the full acceptance-test-harness suite pct-successful This PR has successfully passed the full plugin-compatibility-test suite ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants