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 add/edit description button to the app-bar #9271

Merged
merged 4 commits into from
May 30, 2024

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented May 13, 2024

So far the add/edit description button is rendered on its own line to the far right. This leads to additional space between the description or the title and the following content.

This change moves the add/edit description button to the app-bar where applicable (jobs, runs, computers, users and labels). For views it stays as it is as they don't have a title/app-bar. One also can't just allow to change the description via the configure page as the All View has no configure page.

The change is backwards compatible so plugins using the editableDescription taglib will work as before.

Requires changes in Matrix-project and maven-project plugins to align the new layout

Before/after:

Job:

Before:
image
After:
image
A pipeline job:
image

Run:

Before:
image
After:
image
After clicking the Add Description button
image

User:

Before:
image
After:
image

Computer:

Before:
image
After:
image

Label:

Before:
image
After:
image

Testing done

Manual Testing

Proposed changelog entries

  • Move Add/Edit description to app bar.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

@mawinter69 mawinter69 force-pushed the editable-description branch from 6f45d50 to f91788b Compare May 13, 2024 19:45
So far rhe add/edit description button is rendered as button on its own
line to the far right. This leads to additional space the description or
the title and the following content.

This change moves the add/edit description button to the app-bar where
applicable (jobs, runs, computers, users). For views it stays as it is
as they don't have title/app-bar. One also can't just allow to change
the description via the configure page as the All View has no configure
page.
@mawinter69 mawinter69 force-pushed the editable-description branch from f91788b to f519aed Compare May 14, 2024 12:59
@mawinter69
Copy link
Contributor Author

mawinter69 commented May 14, 2024

Some questions to think about

  • make the add/edit description button a secondary button?
  • should I directly create an overflow button that takes add/edit description?

On the run page

  • also move the keep build forever to overflow?
  • move the info about when the build started and how long it took to the left like e.g. the info about the cause.

image

@mawinter69
Copy link
Contributor Author

@jenkinsci/sig-ux

@NotMyFault NotMyFault added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted needs-security-review Awaiting review by a security team member labels May 18, 2024
@NotMyFault NotMyFault requested a review from a team May 18, 2024 14:34
@timja timja requested a review from janfaracik May 18, 2024 19:58
@janfaracik
Copy link
Contributor

Like it 👍

  • make the add/edit description button a secondary button?

Seems sensible yeah.

  • should I directly create an overflow button that takes add/edit description?

Could do, I do wonder how much benefit/use the 'Add description' button gets to warrant it existing - rather than just existing in the 'Configure' page.

  • also move the keep build forever to overflow?

Sounds good.

  • move the info about when the build started and how long it took to the left like e.g. the info about the cause.

Want to eventually move this section into its own extendable card (in a similar fashion to https://jenkins-redesign.vercel.app/project/build/375). I don't think the current summary components are the best use of space.

@mawinter69
Copy link
Contributor Author

Like it 👍

  • make the add/edit description button a secondary button?

Seems sensible yeah.

  • should I directly create an overflow button that takes add/edit description?

Could do, I do wonder how much benefit/use the 'Add description' button gets to warrant it existing - rather than just existing in the 'Configure' page.

  • also move the keep build forever to overflow?

Sounds good.

  • move the info about when the build started and how long it took to the left like e.g. the info about the cause.

Want to eventually move this section into its own extendable card (in a similar fashion to https://jenkins-redesign.vercel.app/project/build/375). I don't think the current summary components are the best use of space.

For now I only made the description button a secondary button.

- fix permission in job page
- don't show the button for built-in node
- improve jelly doc
@Kevin-CB Kevin-CB 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 May 28, 2024
@Kevin-CB
Copy link
Contributor

LGTM security wise

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 May 28, 2024
@NotMyFault NotMyFault merged commit 4b96f40 into jenkinsci:master May 30, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants