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

[JENKINS-64341] Removed the tbody and table tags for new Jenkins vers… #66

Merged
merged 2 commits into from
Feb 21, 2021

Conversation

ironcerocloudbees
Copy link
Contributor

[JENKINS-64341]

I checked the master version running from a Jenkins 2.279 and I was not able to reproduce the same error described in https://issues.jenkins.io/browse/JENKINS-64341. However, I removed all table/tbody/tr/td/th tag matches for new Jenkins versions where we have the variable divBasedFormLayout.

@ironcerocloudbees
Copy link
Contributor Author

cc @timja, @fqueiruga, @dariver

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.

in this particular issue I wasn't able to reproduce it in isolation but only with a set of plugins, it was quite weird

<d:tag name="blockWrapperTBody">
<j:choose>
<j:when test="${divBasedFormLayout}">
<div>
Copy link
Member

Choose a reason for hiding this comment

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

normally you can just remove this completely tbody is quite uncommon in forms for Jenkins if it works it's fine though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @timja,

Thank you for your review.

So, is it better to remove the tbody tag directly instead of creating and using the blockWrapperTBody?

Copy link
Member

Choose a reason for hiding this comment

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

I think so (but test it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@GLundh
Copy link
Member

GLundh commented Feb 20, 2021

Should I merge this, or does Cloudbees merge PRs themselves in the Jenkins Community?

@timja
Copy link
Member

timja commented Feb 21, 2021

Should I merge this, or does Cloudbees merge PRs themselves in the Jenkins Community?

maintainers of each plugin merge and release

@GLundh GLundh merged commit 56db639 into jenkinsci:master Feb 21, 2021
@fbelzunc
Copy link

fbelzunc commented Mar 3, 2021

@GLundh Hello, do you think it is possible to roll out a release to have this change included?

@GLundh
Copy link
Member

GLundh commented Mar 3, 2021

I'll try to get something done this week.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Belated +1

@GLundh
Copy link
Member

GLundh commented Mar 3, 2021

Release made. I really don't have much time to maintain this plugin anymore. When I picked it up a few years ago, nobody else wanted it and there was some really urgent PRs to be handled. So I stepped in.

Now, since many, many people depends on it, is Cloudbees interested in picking it up?

@oleg-nenashev ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants