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.

Sign up for GitHub

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

Jump to bottom

[FIXED JENKINS-37814] make the icon of the script console configurable #2528

Merged
merged 3 commits into from
Sep 2, 2016
Merged

[FIXED JENKINS-37814] make the icon of the script console configurable #2528

merged 3 commits into from
Sep 2, 2016

Conversation

rpionke
Copy link
Member

@rpionke rpionke commented Aug 30, 2016 •
edited
Loading

JENKINS-37814

discussion of the first PR (reverted) #2522

@oleg-nenashev @Vlatombe how about this approach?

Vlatombe
Vlatombe reviewed Aug 31, 2016
View reviewed changes
core/src/main/resources/lib/hudson/scriptConsole.jelly
<l:layout norefresh="true" permission="${h.RUN_SCRIPTS}">
<st:include page="sidepanel.jelly" />

<l:main-panel>
<h1><img src="${imagesURL}/48x48/${it.icon}" width="48" height="48" alt=""/> ${%Script Console}</h1>
<j:if test="${it.icon == null}">
Copy link
Member

@Vlatombe Vlatombe Aug 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

<j:choose> would be better here.

@oleg-nenashev
Copy link
Member

The approach is a bit unreliable since the plugin providing icon may have 48x48 one.
On the other hand it may have specify full path to icon, which is not equal to imagesURL/48x/48.
So ideally it needs additional changes and integration with the icon class functionality implemented by @tfennelly .

But 👍 since it does not increase the technical debt and fixes the problem stated in JENKINS-37814.

@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Aug 31, 2016
@daniel-beck
Copy link
Member

Is this a recent regression? If so, from when?

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Aug 31, 2016 •
edited
Loading

Maybe d8b1111 (but it would mean it have never worked)

@Vlatombe
Copy link
Member

I agree with Oleg, I think it has never worked

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Sep 1, 2016
@oleg-nenashev oleg-nenashev merged commit a6e1b0b into jenkinsci:master Sep 2, 2016
oleg-nenashev added a commit that referenced this pull request Sep 4, 2016
@oleg-nenashev
Noting #1835, #2532, #2523, #2528 and #2530 towards 2.21
b09043c
olivergondza pushed a commit that referenced this pull request Sep 9, 2016
@olivergondza
[FIXED JENKINS-37814] make the icon of the script console configurable (
feeda1d 
#2528)

* make the icon of the script console configurable

* we don't need an extra variable

* use <j:choose>

(cherry picked from commit a6e1b0b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rpionke @oleg-nenashev @daniel-beck @Vlatombe