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

Add modern icons following new Jenkins UI style #199

Merged
merged 24 commits into from
May 29, 2022
Merged

Add modern icons following new Jenkins UI style #199

merged 24 commits into from
May 29, 2022

Conversation

DuMaM
Copy link
Contributor

@DuMaM DuMaM commented May 24, 2022

I'm using latest Jenkins weekly release. It starts to hurt my eyes when i see those old style icons.
I'm not a graphic designer, but I created them by combining some already made form those sites:
https://github.com/jenkinsci/jenkins/tree/master/war/src/main/resources/images/symbols
https://www.svgrepo.com/

I'm not sure about buildbadge.svg and confighistory.svg, but others I think are ok.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@DuMaM DuMaM requested a review from a team as a code owner May 24, 2022 23:32
@DuMaM DuMaM changed the title Add new modern icons following new Jenkins UI style Add modern icons following new Jenkins UI style May 24, 2022
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.

I agree, the Tango icons are by far no longer up to date. Jenkins core uses ionicons as modern symbols, we probably should do the same here too.

Can you move these icons over to use the symbol API?
Otherwise symbol don't adapt other themes and retain their original color:
Bildschirmfoto 2022-05-25 um 09 42 21

@DuMaM
Copy link
Contributor Author

DuMaM commented May 25, 2022

@NotMyFault could you take a look for it now?
I'm still checking but i do not have experience with jelly so im not sure if those changes are legit.
I also updated all icons to use only ones form ionicons

@DuMaM DuMaM requested a review from NotMyFault May 25, 2022 09:27
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 promising so far, I'll give it a practical test later.

…Consts.java

Co-authored-by: Alexander Brandes <brandes.alexander@web.de>
@DuMaM
Copy link
Contributor Author

DuMaM commented May 25, 2022

Ok, your suggestion probably fix loading config and badge. 😉
I stuck there for some time.
I also change background document symbol to match All configs one.
There is also strange issue, with overflowing two groups. I don't know why 😕
In preview there is no overlapping for filter.svg symbol.
image

@DuMaM
Copy link
Contributor Author

DuMaM commented May 25, 2022

image
image
image
image
Ok it seems to work 🎉, but icons require some tweaks 😭
Also this PR have many breaking changes so until next lts release (with pom.xml upgrade to it), it can't be merged 😢

@DuMaM
Copy link
Contributor Author

DuMaM commented May 25, 2022

BTW do you know why mouse actions do not work?

                          <l:icon src="symbol-info plugin-jobConfigHistory" onmouseOver="showTooltip(${configNr})" onmouseLeave="hideTooltip(${configNr})"  alt="${%Info}" />

@NotMyFault
Copy link
Member

Not too sure about that one, but the oversized icons need a fixed viewbox in these locations, because Jenkins doesn't attempt to resize oversized icons here.

@DuMaM
Copy link
Contributor Author

DuMaM commented May 25, 2022

@DuMaM
Copy link
Contributor Author

DuMaM commented May 25, 2022

@NotMyFault I think i fixed all issues ;)

@DuMaM
Copy link
Contributor Author

DuMaM commented May 25, 2022

Nice catch. I didn't noticed that hyperlinks do not work.

@DuMaM
Copy link
Contributor Author

DuMaM commented May 25, 2022

@NotMyFault I know why this wont work.
It's do to new symbol api. It do not allow for href and tooltips...

https://github.com/jenkinsci/jenkins/blob/2d62032ca422c332b8324ff8925444bd4c9562f9/core/src/main/resources/lib/layout/icon.jelly#L67

@DuMaM
Copy link
Contributor Author

DuMaM commented May 25, 2022

Everything now is working fine 😉
At first i didn't noticed some functionality. Thanks @NotMyFault for suggestions.
image
image
image
image

I'm only wondering why i got an underscore before badge😕

@NotMyFault
Copy link
Member

Bildschirmfoto 2022-05-26 um 10 05 45

It looks like core itself adds a nbsp there, which the a href makes visible, because it's now a clickable URL.

@DuMaM
Copy link
Contributor Author

DuMaM commented May 26, 2022

Are you aware of any href style I should use to remove this underline?

@NotMyFault
Copy link
Member

That'll work for now.

@NotMyFault NotMyFault self-requested a review May 26, 2022 10:36
@DuMaM
Copy link
Contributor Author

DuMaM commented May 26, 2022

@NotMyFault Should we also add this to 'restore' buttons. They are now highlighted with blue.

@NotMyFault
Copy link
Member

@NotMyFault Should we also add this to 'restore' buttons. They are now highlighted with blue.

Yeah, possibly.

pom.xml Show resolved Hide resolved
@NotMyFault NotMyFault merged commit 94c2521 into jenkinsci:master May 29, 2022
@DuMaM DuMaM deleted the feature/make-icons-more-modern branch May 30, 2022 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants