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-44563] Use one-column layout for REST API page #2905

Merged
merged 1 commit into from Jun 3, 2017

Conversation

@recena
Copy link
Contributor

recena commented May 30, 2017

Description

See JENKINS-44563.

Before

jenkins-44564_before

After

jenkins-44563_after2

Changelog entries

Proposed changelog entries:

  • REST API page is using one-column layout

Submitter checklist

  • JIRA issue is well described
  • Link to JIRA ticket in description, if appropriate

Desired reviewers

@jenkinsci/code-reviewers
@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented May 30, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

Copy link
Member

oleg-nenashev left a comment

There are many unrelated changes, and I am not sure it is a good idea to use code tags for non-code text. Needs a separate discussion.

Regarding the type change itself, 👍 from me. Ideally in the column there could be ToC or sidepanel, but it is a separate story.

<h1>REST API</h1>

<p>
Many objects of Jenkins provide the remote access API. They are available
at <tt>${rootURL}/.../api/</tt> where "..." portion is the object for
at <code>${rootURL}/.../api/</code> where "..." portion is the object for

This comment has been minimized.

Copy link
@recena

recena May 30, 2017

Author Contributor

@oleg-nenashev We could consider this pieces of text as code https://developer.mozilla.org/en/docs/Web/HTML/Element/code

</p>
<p>
For XPath that matches multiple nodes, you need to also specify the "wrapper" query parameter
to specify the name of the root XML element to be create so that the resulting XML becomes well-formed.
</p>
<p>
Similarly <tt>exclude</tt> query parameter can be used to exclude nodes
Similarly <code>exclude</code> query parameter can be used to exclude nodes

This comment has been minimized.

Copy link
@recena

recena May 30, 2017

Author Contributor

@oleg-nenashev Maybe we need a special style for reserved words.

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented May 30, 2017

@oleg-nenashev

Regarding the type change itself, 👍 from me. Ideally in the column there could be ToC or sidepanel, but it is a separate story.

Sure. With this layout we could include user-friendly documentation. More space, more flexibiliy, etc...

@jglick
jglick approved these changes May 30, 2017
For more information about remote API in Jenkins, see
<a href="https://jenkins.io/redirect/remote-api">the documentation</a>.
</p>
<p>For more information about remote API in Jenkins, see <a href="https://jenkins.io/redirect/remote-api">the documentation</a>.</p>

This comment has been minimized.

Copy link
@jglick

jglick May 30, 2017

Member

Why this formatting change?

This comment has been minimized.

Copy link
@recena

recena May 30, 2017

Author Contributor

readability?

This comment has been minimized.

Copy link
@jglick

jglick May 30, 2017

Member

Just please avoid touching unrelated lines of code when filing a PR. The longer the diff, the harder it is to review, thus the less likely it is to be merged (and the more likely your change is to cause problems for merging other changes, cherry-picking fixes to LTS, blameing, etc.).

This comment has been minimized.

Copy link
@recena

recena May 30, 2017

Author Contributor

@jglick I don't believe this is such case. This file has had (and has) very slow activity, I don't see the problem. At any case, changes related with the UI, are susceptible of resulting complex for reviewing, for that reason, I try to provide screenshots and manual tests.

This comment has been minimized.

Copy link
@recena

recena May 30, 2017

Author Contributor

And yes, I believe this project needs to review the code-formatting of some files 😄

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck May 30, 2017

Member

I believe this project needs to review the code-formatting of some files

Including unrelated changes with a PR is not the way to do that.

This comment has been minimized.

Copy link
@recena

recena May 31, 2017

Author Contributor

@daniel-beck @jglick These small style-format changes are blocking this PR?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 31, 2017

Member

@recena Integrating obviously unrelated changes into a single PR slows down review and merge of the real fix. It is convenient neither for reviews nor for the committer himself. Any reviewer may legitimately block the PR due to unrelated changes, so by adding them you accept the risk of such blockage. Just IMHO.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck May 31, 2017

Member

@recena It's not a big deal if it happens occasionally. We also don't expect you to not do an obvious improvement near the code you need to change anyway (let's say "near" is "git diff doesn't skip unmodified lines in between" as a rule of thumb). But IIRC inclusion of unrelated changes, even in otherwise unchanged files, has been a recurring issue in PRs from you. We bring it up every time it happens, but you don't internalize our feedback. In fact, it looks deliberate, despite being asked not to do it.

What would you expect us to do in this situation?

This comment has been minimized.

Copy link
@recena

recena May 31, 2017

Author Contributor

@daniel-beck Again. Is it blocking the PR merge? If so, I'll revert the changes.

@@ -24,13 +24,13 @@ THE SOFTWARE.

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt">
<l:layout title="Remote API">
<l:main-panel>
<l:layout title="Remote API" type="one-column">

This comment has been minimized.

Copy link
@jglick

jglick May 30, 2017

Member

This is the only part of the diff actually implementing the stated change IIUC.

@@ -38,8 +38,7 @@ THE SOFTWARE.
<h2>Fetch/Update job description</h2>
<p>
<a href="../description">this URL</a>
can be used to get and set just the job description. POST form data with a
"description" parameter to set the description.
can be used to get and set just the job description. POST form data with a "description" parameter to set the description.

This comment has been minimized.

Copy link
@jglick

jglick May 30, 2017

Member

Ditto—gratuitous diff AFAICT.

This comment has been minimized.

Copy link
@recena

recena May 30, 2017

Author Contributor

@jglick Do you prefer a separate PR for applying some of style-format?

@kuisathaverat

This comment has been minimized.

Copy link
Contributor

kuisathaverat commented May 31, 2017

nice improvement 👍

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented May 31, 2017

@kuisathaverat Thanks for your feedback.

@recena recena force-pushed the recena:JENKINS-44563 branch from fdc6dd3 to 4fc5825 May 31, 2017
@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented May 31, 2017

@oleg-nenashev @jglick @daniel-beck Better?

Could I prepare a new PR for replacing old-school HTML markup?

@@ -151,5 +151,5 @@ THE SOFTWARE.

<st:include it="${it.bean}" page="_api.jelly" optional="true" />
</l:main-panel>
</l:layout>
</l:layout>

This comment has been minimized.

Copy link
@recena

recena May 31, 2017

Author Contributor

Removing tab characters...

@jglick
jglick approved these changes May 31, 2017
Copy link
Member

jglick left a comment

FWIW I was not blocking the PR before—I approved it—but this is indeed better, thanks.

A PR replacing tt with code would be fine. I would generally object to a PR which just reformats code for fun, unless the original condition was truly unreadable (which is certainly not the case here).

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented May 31, 2017

@jglick Thanks so much.

I would generally object to a PR which just reformats code for fun, unless the original condition was truly unreadable (which is certainly not the case here).

I never do changes for fun 😄

@recena recena closed this Jun 1, 2017
@recena recena reopened this Jun 1, 2017
@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Jun 1, 2017

The PR builder is failing. Unrelated at all.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jun 2, 2017

@reviewbybees done I'd guess

@daniel-beck daniel-beck merged commit aff0b92 into jenkinsci:master Jun 3, 2017
1 check passed
1 check passed
continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.