Skip to content

Optional background images for job statuses#18

Open
smurf667 wants to merge 5 commits into
jenkinsci:masterfrom
smurf667:master
Open

Optional background images for job statuses#18
smurf667 wants to merge 5 commits into
jenkinsci:masterfrom
smurf667:master

Conversation

@smurf667

@smurf667 smurf667 commented Aug 9, 2015

Copy link
Copy Markdown

Hello, this feature allows to show an optional and configurable background image per job status.

@jenkinsadmin

Copy link
Copy Markdown
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why that? Are you trying to prevent autoboxing or something?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I am. This is just a matter of preference - I'm no fan of autoboxing...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, I just feel this is a bit more verbose in that case. Anyway :).

Back to the very goal of that PR itself, please revert those changes as they're unrelated to the newly introduced feature, so that the code change only contains the part related to the addition.

If you really wish to replace those, please file another dedicated PR.

Thanks!

@batmat

batmat commented Jan 22, 2016

Copy link
Copy Markdown
Member

Hi @smurf667, thanks for your work!

As is, and also because the existing code was not already perfect in the regard, I would be reluctant to add and manage again more scalar attributes.

I think the existinig code and the PR should/could be reworked to avoid adding and conveying yet again 5 more attributes/parameters.

Thanks again!

*/
public class RadiatorView extends ListView {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

whitespace modification

batmat added a commit that referenced this pull request Feb 15, 2016
Goal: be able to use DataBoundSetter to try and clean the currently
bloated constructor (and many PR still open wanting to open yet many
other new attributes :-/).

See https://groups.google.com/forum/#!topic/jenkinsci-dev/58-DEvuJZWI :
this new feature requires 1.535 (so choosing the next LTS above that).

See also:
* #18
* #17
@batmat

batmat commented Feb 20, 2016

Copy link
Copy Markdown
Member

Hi @smurf667 I've committed 14403e5 that uses @DataBoundSetter to avoid having a bunch of parameters in the constructor.

Could you please:

  • rebase on the master tip and use the same principle to use the same "pattern"?
  • squash your multiple commits this PR so that commits are more meaningful (basically, squash/fixup any commit whose role is only to fix a previous one of the PR)

Then I'll have a look again.

(I'd still like to improve the code to use richer objects and aggregate related things, but that an issue outside the scope of your PR so I'll do that later if I ever find the time to do it.)

@smurf667

Copy link
Copy Markdown
Author

Hi,
I've adapted the code, but my Git-foo is not sufficient for the squashing. The commits appear meaningful to me anyway. Hope that is good enough. Otherwise I'll close the pull request as I can't spend more time on this. Thanks!

@batmat batmat self-requested a review February 10, 2018 10:29
@batmat batmat removed their request for review November 14, 2018 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants