[JENKINS-69549] Margins for headers and paragraphs make descriptions … #7078

frankie139506 · 2022-09-07T14:00:48Z

See JENKINS-69549.
Before:

After:

Another example:

Proposed changelog entries

Reduce spacing in help files

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content-Security-Policy directives (see documentation on jenkins.io).
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

…and non-trivial inline help look terrible

daniel-beck · 2022-09-07T20:09:13Z

With changes like this it would be interesting to know why the original change was done in the first place. Requesting @janfaracik as a reviewer to point out what this change in turn will break ;-)

janfaracik · 2022-09-08T20:43:18Z

With changes like this it would be interesting to know why the original change was done in the first place. Requesting @janfaracik as a reviewer to point out what this change in turn will break ;-)

The original change was made to make page headers have a margin consistent with the page margin. This causes issues like the above however.

For https://issues.jenkins.io/browse/JENKINS-69549 there's two issues:

the titles are using h1 - this should be changed to be correct for the heading level
the margin is set incorrectly (as mentioned above)

(I haven't tried this PR yet) but I believe a better fix would be to change the value of --section-padding in the help component class, e.g. half the current value.

…and non-trivial inline help look terrible

frankie139506 · 2022-09-09T01:32:02Z

the titles are using h1 - this should be changed to be correct for the heading level
I have changed to h3 for the title.

the margin is set incorrectly (as mentioned above)
I have made some change and I think this change is more reasonable.

daniel-beck · 2022-09-09T06:54:08Z

the titles are using h1 - this should be changed to be correct for the heading level

This is impractical to implement across the ecosystem. We need to expect that all levels of h*, p, div, and basic formatting are used freely in help files, and deal with it.

janfaracik · 2022-09-09T08:43:43Z

the titles are using h1 - this should be changed to be correct for the heading level

This is impractical to implement across the ecosystem. We need to expect that all levels of h*, p, div, and basic formatting are used freely in help files, and deal with it.

Would it be possible (and relatively straightforward) to dynamically change the header levels when clicking the❓button?

daniel-beck · 2022-09-09T11:31:32Z

Would it be possible (and relatively straightforward) to dynamically change the header levels when clicking the❓button?

We already rewrite the responses some in

jenkins/war/src/main/webapp/scripts/hudson-behavior.js

Lines 990 to 1005 in 04b46d1

    
             // Ensure links open in new window unless explicitly specified otherwise 
        
             var links = div.getElementsByTagName("a"); 
        
             for (var i = 0; i < links.length; i++) { 
        
               var link = links[i]; 
        
               if (link.hasAttribute("href")) { 
        
                 // ignore document anchors 
        
                 if (!link.hasAttribute("target")) { 
        
                   link.setAttribute("target", "_blank"); 
        
                 } 
        
                 if (!link.hasAttribute("rel")) { 
        
                   link.setAttribute("rel", "noopener noreferrer"); 
        
                 } 
        
               } 
        
             } 
        
             layoutUpdateCallback.call(); 
        
           },

so it should be feasible. Is this preferable to e.g. having styles conditional on h* being inside/output the help block, and if so, why?

war/src/main/less/base/style.less

github-actions · 2023-04-21T07:37:04Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

timja · 2024-10-15T21:27:44Z

the titles are using h1 - this should be changed to be correct for the heading level

This is impractical to implement across the ecosystem. We need to expect that all levels of h*, p, div, and basic formatting are used freely in help files, and deal with it.

I've checked and usage statistics was the only place in the ecosystem using an h1:
https://github.com/search?q=org%3Ajenkinsci+h1+language%3AXML+path%3A%2F.%2Bhelp-.%2B%2F&type=code

So no need to rewrite the response imo.

timja · 2024-10-15T21:29:27Z

src/main/scss/base/_style.scss

@@ -389,6 +391,24 @@ pre.console {
  p:last-of-type {


I tried removing this as its the cause of the heading spacing issue but I couldn't find a way to make it work whilst keeping no spacing at the bottom of the help file (made worse in help from plugins that have from plugin ... as the space was a bit excessive without this.

timja · 2024-10-15T21:29:52Z

src/main/scss/base/_style.scss

+  // add spacing above headings except for when its the first element in the help
+  // the need for this is caused by p:last-of-type setting margin-bottom to 0
+  // unfortunately because of the varied markup I wasn't able to find a way to avoid this
+  h1:not(:first-child),


@janfaracik what do you think about this approach and can you think of a simpler one?

Yeah I can't think of anything sadly, could use 'gap' on the parent container but due to how varied the contents are it probably would make things worse :/

timja · 2024-10-15T21:31:08Z

core/src/main/resources/hudson/model/UsageStatistics/global.groovy

@@ -8,7 +8,7 @@ def f=namespace(lib.FormTagLib)

 f.section(title: _("Usage Statistics")) {
    if (UsageStatistics.DISABLED) {
-        span(class: "jenkins-not-applicable") {
+        div(class: "jenkins-not-applicable jenkins-!-margin-bottom-2") {


Didn't look great as it was touching the line

Before:

After:

timja · 2024-10-15T21:31:41Z

core/src/main/resources/hudson/model/UsageStatistics/help-usageStatisticsCollected.jelly

@@ -38,24 +38,34 @@
            Each trial has a specific purpose and a defined end date, after which collection stops, independent of the installed versions of Jenkins or plugins.
            Once a trial is complete, the trial results may be aggregated and shared with the developer community.
        </p>
-        <p>
-            The following trials defined on this instance are active now or in the future:


the help was ending with this as there's no trials currently (and an empty dl)

janfaracik · 2024-10-22T10:06:56Z

Will take a look 👍

timja · 2024-12-04T15:14:18Z

@janfaracik please take a look

core/src/main/resources/hudson/model/UsageStatistics/global.groovy

…oovy

janfaracik · 2024-12-07T19:25:34Z

src/main/scss/base/_style.scss

+  // add spacing above headings except for when its the first element in the help
+  // the need for this is caused by p:last-of-type setting margin-bottom to 0
+  // unfortunately because of the varied markup I wasn't able to find a way to avoid this
+  h1:not(:first-child),


Yeah I can't think of anything sadly, could use 'gap' on the parent container but due to how varied the contents are it probably would make things worse :/

timja · 2024-12-07T21:07:03Z

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

[JENKINS-69549] Margins for headers and paragraphs make descriptions …

bd1e296

…and non-trivial inline help look terrible

daniel-beck requested a review from janfaracik September 7, 2022 20:08

basil mentioned this pull request Sep 7, 2022

Polish forms components and split out CSS files #6259

Merged

11 tasks

[JENKINS-69549] Margins for headers and paragraphs make descriptions …

4ad77a9

…and non-trivial inline help look terrible

Merge branch 'master' into JENKINS-69549

f82328f

NotMyFault added regression-fix Pull request that fixes a regression in one of the previous Jenkins releases web-ui The PR includes WebUI changes which may need special expertise labels Sep 14, 2022

NotMyFault reviewed Sep 14, 2022

View reviewed changes

war/src/main/less/base/style.less Outdated Show resolved Hide resolved

NotMyFault requested a review from a team September 14, 2022 20:55

Update style.less

e9227a6

github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 21, 2023

timja added 2 commits October 15, 2024 19:52

Merge branch 'master' into JENKINS-69549

08264c1

Address feedback

bebc28a

github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 15, 2024

Revert change while testing

55f970c

timja requested review from janfaracik and NotMyFault and removed request for janfaracik October 15, 2024 21:26

timja reviewed Oct 15, 2024

View reviewed changes

timja approved these changes Oct 15, 2024

View reviewed changes

timja requested review from a team and daniel-beck October 15, 2024 21:32

Merge branch 'master' into JENKINS-69549

b890bed

janfaracik reviewed Dec 7, 2024

View reviewed changes

core/src/main/resources/hudson/model/UsageStatistics/global.groovy Outdated Show resolved Hide resolved

Update core/src/main/resources/hudson/model/UsageStatistics/global.gr…

07d8996

…oovy

janfaracik approved these changes Dec 7, 2024

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 7, 2024

timja merged commit f6802dd into jenkinsci:master Dec 9, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-69549] Margins for headers and paragraphs make descriptions … #7078

[JENKINS-69549] Margins for headers and paragraphs make descriptions … #7078

frankie139506 commented Sep 7, 2022 •

edited by timja

Loading

daniel-beck commented Sep 7, 2022

janfaracik commented Sep 8, 2022

frankie139506 commented Sep 9, 2022 •

edited

Loading

daniel-beck commented Sep 9, 2022 •

edited

Loading

janfaracik commented Sep 9, 2022 •

edited

Loading

daniel-beck commented Sep 9, 2022

github-actions bot commented Apr 21, 2023

timja commented Oct 15, 2024

timja Oct 15, 2024

timja Oct 15, 2024

janfaracik Dec 7, 2024

timja Oct 15, 2024

timja Oct 15, 2024

janfaracik commented Oct 22, 2024

timja commented Dec 4, 2024

janfaracik Dec 7, 2024

timja commented Dec 7, 2024

[JENKINS-69549] Margins for headers and paragraphs make descriptions … #7078

[JENKINS-69549] Margins for headers and paragraphs make descriptions … #7078

Conversation

frankie139506 commented Sep 7, 2022 • edited by timja Loading

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

daniel-beck commented Sep 7, 2022

janfaracik commented Sep 8, 2022

frankie139506 commented Sep 9, 2022 • edited Loading

daniel-beck commented Sep 9, 2022 • edited Loading

janfaracik commented Sep 9, 2022 • edited Loading

daniel-beck commented Sep 9, 2022

github-actions bot commented Apr 21, 2023

timja commented Oct 15, 2024

timja Oct 15, 2024

Choose a reason for hiding this comment

timja Oct 15, 2024

Choose a reason for hiding this comment

janfaracik Dec 7, 2024

Choose a reason for hiding this comment

timja Oct 15, 2024

Choose a reason for hiding this comment

timja Oct 15, 2024

Choose a reason for hiding this comment

janfaracik commented Oct 22, 2024

timja commented Dec 4, 2024

janfaracik Dec 7, 2024

Choose a reason for hiding this comment

timja commented Dec 7, 2024

frankie139506 commented Sep 7, 2022 •

edited by timja

Loading

frankie139506 commented Sep 9, 2022 •

edited

Loading

daniel-beck commented Sep 9, 2022 •

edited

Loading

janfaracik commented Sep 9, 2022 •

edited

Loading