Skip to content

Fix help display behaviors in the trait list UI - #328

Merged
jglick merged 6 commits into
jenkinsci:masterfrom
dodo920306:fix/help-display-ui-behaviors
Oct 3, 2025
Merged

jglick merged 6 commits into
jenkinsci:masterfrom
dodo920306:fix/help-display-ui-behaviors

Conversation

@dodo920306

Copy link
Copy Markdown
Contributor

Wrap helpLink and helpArea in a .help-sibling container to ensure help content appears under the correct entry, rather than always in the first block. Also moved the helpLink inline with displayName inside the same to improve layout.

  • Fixes confusing UX in repeatable entries
  • Matches Jenkins' help JavaScript behavior (finds closest .help-sibling)

Before:
ezgif-8b255e49809e91

After:
ezgif-8b01f6aaa1c984

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

Wrap helpLink and helpArea in a `.help-sibling` container to ensure help content
appears under the correct entry, rather than always in the first block. Also moved
the helpLink inline with displayName inside the same <td> to improve layout.

- Fixes confusing UX in repeatable entries
- Matches Jenkins' help JavaScript behavior (finds closest `.help-sibling`)
- Removes unnecessary <div> wrapping displayName
@dodo920306
dodo920306 requested a review from a team as a code owner May 19, 2025 12:18
@dodo920306

Copy link
Copy Markdown
Contributor Author

@jenkinsci/scm-api-plugin-developers Any update?

</local:td>
<f:helpLink url="${help}"/>
</local:row>
<!-- TODO: help support is unintuitive; people should be able to see help from drop-down menu -->

@jglick jglick Sep 17, 2025 •

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.

dates to #36 FTR; amended in #82

@jglick
jglick requested a review from timja September 17, 2025 22:48
@jglick

jglick commented Sep 17, 2025

Copy link
Copy Markdown
Member

@timja any idea if this help-sibling was ever intended as an API?

The whole file seems rather suspicious to me. You should not need all of this boilerplate just to add a list of traits to an object, something that can be done much more easily with the standard f:repeatableHeteroProperty as for example in jenkinsci/oic-auth-plugin#644. I think the only thing we get out of this complexity is the visual categorization of traits into sections.

@timja

timja commented Sep 18, 2025

Copy link
Copy Markdown
Member

This change itself seems fine, much better that it is on the same line.

@timja any idea if this help-sibling was ever intended as an API?

Ideally it should not be used outside of core to reduce the changes needed to be made in the ecosystem if / when the markup ever changes, it predates public APIs vs internal for CSS in core though.

It is used in a number of places outside core: https://github.com/search?q=org%3Ajenkinsci+help-sibling&type=code

The whole file seems rather suspicious to me. You should not need all of this boilerplate just to add a list of traits to an object, something that can be done much more easily with the standard f:repeatableHeteroProperty as for example in jenkinsci/oic-auth-plugin#644. I think the only thing we get out of this complexity is the visual categorization of traits into sections.

It would be better is the section support could be added to core and then removed from here, not looked into the feasibility of that though.

@dodo920306

Copy link
Copy Markdown
Contributor Author

So...should I change anything?

Comment on lines +68 to +71
<local:td>
<b>${descriptor.displayName}</b>
<f:helpLink url="${help}"/>
</local:td>

@timja timja Sep 24, 2025 •

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.

This wasn't merged properly with #338:

Suggested change
<local:td>
<b>${descriptor.displayName}</b>
<f:helpLink url="${help}"/>
</local:td>
${descriptor.displayName}
<f:helpLink url="${help}"/>

Interestingly local:td doesn't exist and its being being outputted directly to the html as:

<local:td xmlns:local="local">
...

@timja timja left a comment •

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.

I just checked with master and this was fixed in #338, these changes aren't required.

cc @janfaracik

@jglick

jglick commented Oct 1, 2025

Copy link
Copy Markdown
Member

#328 (review)

@jglick jglick closed this Oct 1, 2025
@dodo920306

Copy link
Copy Markdown
Contributor Author

@jglick @timja Wait. What? You can't close this. The problem still remains even with the latest version of this plugin!
ezgif-66256d0d19fd86
image

I thought #328 (review) was talking about the change in the review rather than this whole PR. Anyway, please reopen this 🙏.

@timja

timja commented Oct 2, 2025

Copy link
Copy Markdown
Member

In my testing it did not exist anymore. The video you have attached shows no issue that I can see?

@dodo920306

dodo920306 commented Oct 2, 2025 •

Copy link
Copy Markdown
Contributor Author

In my testing it did not exist anymore. The video you have attached shows no issue that I can see?

@timja Well, in the first sentence of my PR I said

Wrap helpLink and helpArea in a .help-sibling container to ensure help content appears under the correct entry, rather than always in the first block.

No matter what you originally thought this PR tried to resolve, the main issue I want to address here is the problem that the help messages don't show in their own option block and can't be shown at the same time separately.

The correct behavior after this PR got merged should be like the GIF I showed in the PR
ezgif-8b01f6aaa1c984

In my testing it did not exist anymore. The video you have attached shows no issue that I can see?

The problen of that video is when I clicked another option help message while the other is still opened, the original message got closed with the message I want to see only open in the first entry after I clicked again. I believe it's more intuitive to make help messages show wherever and whenever users click.

@timja timja reopened this Oct 2, 2025
<d:tag name="body">
<div>
<j:set var="help" value="${descriptor.helpFile}"/>
<div class="help-sibling" style="width:100%">

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.

I doubt this style is needed:

Suggested change
<div class="help-sibling" style="width:100%">
<div class="help-sibling">

@timja

timja commented Oct 2, 2025

Copy link
Copy Markdown
Member

I see, I misunderstood that there is two bugs here, the display of the help icons is fixed but there's an issue with opening and closing of the help areas.

If you can resolve the comments I'll take another look

Signed-off-by: dodo920306 <dodo920306@gmail.com>
@dodo920306

dodo920306 commented Oct 3, 2025 •

Copy link
Copy Markdown
Contributor Author

Like this, you meant?

Comment thread src/main/resources/jenkins/scm/api/form/traits.jelly Outdated
@timja

timja commented Oct 3, 2025

Copy link
Copy Markdown
Member

Like this, you meant?

Mostly, I fixed the merge for you in a2b65e1

@timja timja added the bug label Oct 3, 2025

@timja timja left a comment

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.

Thanks! I tested on master and reproduced the issue and can no longer reproduce it with this fix.

@timja
timja requested a review from jglick October 3, 2025 13:34

@jglick jglick left a comment

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.

Final diff does not appear harmful. Do not know anything about the bug or the fix. @timja maybe you want to be added to the repo so you can help maintain stuff like this yourself?

@jglick
jglick added this pull request to the merge queue Oct 3, 2025
Merged via the queue into jenkinsci:master with commit ae7ca4d Oct 3, 2025
17 checks passed
@dodo920306
dodo920306 deleted the fix/help-display-ui-behaviors branch October 7, 2025 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants