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

Update buttons to use new classes on the dashboard #6799

Merged
merged 45 commits into from
Aug 15, 2022

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Jul 7, 2022

Small PR to update the design of the .jenkins-button classes as well as to use the classes on the dashboard. This is the first part of several PRs to update buttons across Jenkins (see the closed #5897 for a rough end goal).

Before
image

After
image

It's rather hard to tell any major differences here (apart from color), the key differences are the hover/focus states:

Before

Screen.Recording.2022-07-07.at.14.56.34.mov

After

Screen.Recording.2022-07-07.at.14.58.34.mov

New keyboard focus state
Screenshot 2022-07-07 at 14 59 35

What else?

  • Slight change to .jenkins-button appearance (slightly more rounded/padded, icons are larger)
  • Semantic modifiers have been removed (e.g. .jenkins-button--destructive), you can now use semantic and palette classes to the same thing (e.g. .jenkins-button .jenkins-!-destructive-color)
  • .jenkins-button--transparent has been renamed to .jenkins-button--tertiary

Seeing as .jenkins-button isn't used anywhere apart from Jenkins/Design Library it should be fine to make these breaking changes without backwards compatibility. I'll update Design Library to support these changes.

Proposed changelog entries

  • Update design of buttons on the dashboard

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jenkinsci/sig-ux

Maintainer checklist

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

  • There are at least 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 does not block the change
  • Changelog entries in the PR 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, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (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 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 Jul 11, 2022
@NotMyFault
Copy link
Member

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.

Looks nice 👏🏻

@NotMyFault NotMyFault requested a review from a team July 11, 2022 21:42
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.

Likely intentional but I wanted to check before approving.


The icon size buttons don't match the screenshots you've provided.

image

the active one is faded out while the others show as more button like.

is it meant to be like that?

@janfaracik
Copy link
Contributor Author

Likely intentional but I wanted to check before approving.

The icon size buttons don't match the screenshots you've provided.

image

the active one is faded out while the others show as more button like.

is it meant to be like that?

Not intentional - Might need to clear cache there, your screenshot is showing the old style buttons. I've just reran it again on my end and it produces what's seen in the screenshot -

Screenshot 2022-07-14 at 09 49 48

@timja
Copy link
Member

timja commented Jul 14, 2022

Ok clearing cache worked, for me clicking the button size changes doesn't seem to work.

it worked for one click on S but then I couldn't get back to L, when I cleared by cookies I can't change the size any more

@janfaracik
Copy link
Contributor Author

Ok clearing cache worked, for me clicking the button size changes doesn't seem to work.

it worked for one click on S but then I couldn't get back to L, when I cleared by cookies I can't change the size any more

Can't seem to reproduce that in Safari or Chrome.

Screen.Recording.2022-07-14.at.18.28.39.mov

@timja
Copy link
Member

timja commented Jul 14, 2022

Can't seem to reproduce that in Safari or Chrome.

How are you testing? I'm using mvn jetty:run from https://github.com/jenkinsci/jenkins/blob/master/CONTRIBUTING.md#building-and-debugging

If I request the URL http://localhost:8080/jenkins/iconSize?24x24 and I have a breakpoint at

String qs = req.getQueryString();
the breakpoint is never called.

If I add a trailing slash then it's correctly invoked:
http://localhost:8080/jenkins/iconSize/?24x24


This seems to be a pre-existing issue and I can reproduce it on master, would be good to hear someone else can as well though.

@janfaracik
Copy link
Contributor Author

https://github.com/jenkinsci/jenkins/blob/master/CONTRIBUTING.md#building-and-debugging

I'm doing the same but can't seem to reproduce it on master either. Do you have any projects/folders in the table when it isn't working?

@timja
Copy link
Member

timja commented Jul 18, 2022

I'm doing the same but can't seem to reproduce it on master either. Do you have any projects/folders in the table when it isn't working?

A bunch, including some with strange names for CSRF, I'll try from a clean one later on

@janfaracik
Copy link
Contributor Author

I'm doing the same but can't seem to reproduce it on master either. Do you have any projects/folders in the table when it isn't working?

A bunch, including some with strange names for CSRF, I'll try from a clean one later on

Is this PR good to move forward without that?

@timja
Copy link
Member

timja commented Aug 4, 2022

Yes (as I could reproduce on master as well)

@NotMyFault NotMyFault requested a review from timja August 12, 2022 09:13
@timja
Copy link
Member

timja commented Aug 12, 2022

/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 12, 2022
@timja timja merged commit cb232e8 into jenkinsci:master Aug 15, 2022
@timja timja deleted the new-buttons-1-dashboard branch August 15, 2022 21:07
@ViliusS
Copy link

ViliusS commented Aug 17, 2022

Please stop taking out color out of design elements. These buttons were blue to indicate that they are clickable. What's separating them now from the rest of the text?

@daniel-beck
Copy link
Member

Probably causes JENKINS-70247.

@basil
Copy link
Member

basil commented Jan 1, 2023

Probably causes JENKINS-70247.

Confirmed through bisection.

This pull request was closed.
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.

7 participants