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 the 'Update' and 'Install' buttons to the app bar #8025

Merged
merged 13 commits into from Jul 10, 2023

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented May 21, 2023

Screen.Recording.2023-05-21.at.01.25.34.mov
image

This PR removes the bottom app bar from the Plugins pages, moving the final control to the top app bar. This increases the available screen real estate for updates. The 'Update' and 'Install' buttons will now be disabled unless a plugin is checked, whereas previously you could click it and it would do nothing.

Testing done

  • Plugins install/update as before
  • The 'Update' and 'Install' buttons enable/disable depending on if an update is checked

Proposed changelog entries

  • Move the 'Update' and 'Install' buttons to the app bar.

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@jenkinsci/sig-ux

Maintainer checklist

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

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@NotMyFault NotMyFault 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 May 21, 2023
Copy link
Contributor

@StefanSpieker StefanSpieker left a comment

Choose a reason for hiding this comment

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

I really like this, I'm just not sure if the download and install directly functionally is still needed for updates. In the install tab it is still present?

<div class="bottom-sticker-inner">
<l:isAdmin>
<j:if test="${!empty(list)}">
<f:submit value="${%Download now and install after restart}"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this functionally removed?

@mawinter69
Copy link
Contributor

The change only affects the plugin updates page
What about the available plugins page? It would be inconsistent to have the buttons there still at the bottom

@janfaracik
Copy link
Contributor Author

janfaracik commented May 22, 2023

The change only affects the plugin updates page
What about the available plugins page? It would be inconsistent to have the buttons there still at the bottom

Happy to make that change as part of this PR or a quick follow up PR. Any thoughts?

@timja
Copy link
Member

timja commented May 22, 2023

The change only affects the plugin updates page
What about the available plugins page? It would be inconsistent to have the buttons there still at the bottom

Happy to make that change as part of this PR or a quick follow up PR. Any thoughts?

the PR is quite small unless updating that will change a lot I think better to do the plugin manager as a whole

commit abd50ba
Merge: d828392 31bb443
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun May 21 01:35:23 2023 +0100

    Merge branch 'restyle-footer' of https://github.com/janfaracik/jenkins into restyle-footer

commit d828392
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun May 21 01:34:46 2023 +0100

    Require SYSTEM_READ to show About Jenkins option

commit 9c79f30
Merge: 3007eae a74425c
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun May 21 01:34:34 2023 +0100

    Merge branch 'master' into restyle-footer

commit 31bb443
Merge: 3007eae a74425c
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun May 21 01:31:51 2023 +0100

    Merge branch 'master' into restyle-footer

commit 3007eae
Author: Alexander Brandes <mc.cache@web.de>
Date:   Sat May 13 10:46:42 2023 +0200

    Fix linting

commit 66f7d3d
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri May 12 20:15:05 2023 +0100

    Init
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jun 7, 2023
@github-actions
Copy link

github-actions bot commented Jun 7, 2023

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jun 7, 2023
@janfaracik
Copy link
Contributor Author

I've updated it to include the buttons -

image

It's a dropdown button (due to a lack of space on smaller screens). Happy to change it into a split button if people prefer.

@mawinter69
Copy link
Contributor

I guess the majority of users select Install without restart so from that point of view a split button might be better.

@janfaracik janfaracik changed the title Move the 'Update' button to the app bar Move the 'Update' and 'Install' buttons to the app bar Jun 10, 2023
@janfaracik
Copy link
Contributor Author

I guess the majority of users select Install without restart so from that point of view a split button might be better.

image

Updated to be a split button. Thanks for the feedback :)

@StefanSpieker
Copy link
Contributor

That looks really nice, but the CI complains about prettier:

[2023-06-10T19:22:42.471Z] [INFO] Running 'yarn lint:ci' in /home/jenkins/agent/workspace/Core_jenkins_PR-8025/war
[2023-06-10T19:22:47.533Z] [INFO] Checking formatting...
[2023-06-10T19:22:53.261Z] [INFO] [warn] src/main/js/plugin-manager-ui.js
[2023-06-10T19:22:58.008Z] [INFO] [warn] Code style issues found in the above file. Forgot to run Prettier?

@timja timja requested a review from a team June 16, 2023 07:34
timja added a commit to jenkinsci/acceptance-test-harness that referenced this pull request Jun 16, 2023
@NotMyFault
Copy link
Member

I guess the majority of users select Install without restart so from that point of view a split button might be better.

image Updated to be a split button. Thanks for the feedback :)

Clicking on the button does nothing for me.

@timja
Copy link
Member

timja commented Jun 16, 2023

Same I was just testing it and the button appears to be broken. (The updates button works and looks great)

@timja
Copy link
Member

timja commented Jun 18, 2023

(re-tested and looks great, waiting on ATH)

@NotMyFault NotMyFault requested a review from a team June 26, 2023 15:57
@NotMyFault NotMyFault added the needs-security-review Awaiting review by a security team member label Jun 26, 2023
Copy link
Contributor

@yaroslavafenkin yaroslavafenkin left a comment

Choose a reason for hiding this comment

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

Looks fine security wise

@yaroslavafenkin yaroslavafenkin 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 Jun 27, 2023
@NotMyFault NotMyFault added the ath-fail The acceptance-test-harness suite needs a forward-compatible change label Jun 30, 2023
@NotMyFault NotMyFault added ath-successful This PR has successfully passed the full acceptance-test-harness suite and removed ath-fail The acceptance-test-harness suite needs a forward-compatible change labels Jul 7, 2023
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Thanks!

@NotMyFault NotMyFault requested a review from a team July 7, 2023 18:08
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/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 Jul 7, 2023
@NotMyFault NotMyFault merged commit fbf1e27 into jenkinsci:master Jul 10, 2023
16 checks passed
@janfaracik janfaracik deleted the plugins-update-move-to-app-bar branch July 10, 2023 10:31
<div class="jenkins-split-button">
<button id="button-install" form="form" type="submit" name="dynamicLoad" class="jenkins-button jenkins-button--primary">
<div class="jenkins-dropdown__item__icon">
<l:icon src="symbol-download" />
Copy link
Member

Choose a reason for hiding this comment

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

using download icon to me is confusing as I (the user) am not downloading anything to my machine.
Rather Jenkins is installing - which is normally an arrow pointing right or the like.

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 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
7 participants