Skip to content

[JENKINS-73961] Extract event handlers in BuildPipelineView/bpp.jelly - #144

Open
shlomomdahan wants to merge 9 commits into
jenkinsci:masterfrom
shlomomdahan:JENKINS-73961
Open

[JENKINS-73961] Extract event handlers in BuildPipelineView/bpp.jelly#144
shlomomdahan wants to merge 9 commits into
jenkinsci:masterfrom
shlomomdahan:JENKINS-73961

Conversation

@shlomomdahan

@shlomomdahan shlomomdahan commented Oct 30, 2024

Copy link
Copy Markdown
Contributor

JENKINS-73961

Manually tested and is working correctly.

Testing done

BEFORE CHANGES
AFTER CHANGES

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 demonstrates feature works or fixes the issue

@shlomomdahan
shlomomdahan marked this pull request as ready for review October 30, 2024 22:58
@shlomomdahan
shlomomdahan requested a review from a team as a code owner October 30, 2024 22:58
@shlomomdahan shlomomdahan mentioned this pull request Nov 4, 2024
6 tasks
<div class="icon-container">
<j:if test="${from.isProjectParameterized()}">
<span class="pointer" onclick="buildPipeline.fillDialog('${app.rootUrl}${from.gridBuilder.firstJobLink}/build?delay=0sec', 'Starts the pipeline with parameters')">
<span class="bpp-trigger-pipeline-parameterized-params" data-fill-dialog-href="${app.rootUrl}${from.gridBuilder.firstJobLink}/build?delay=0sec" data-fill-dialog-title="Starts the pipeline with parameters" style="display: none" />

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 the need for a different element to hold the data attributes? Is it not simpler to put them on the element on the next line?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it - I made the changed and tested that it is working. I'll note all things things for future PR to avoid bloating the DOM.

Thank you!

@yaroslavafenkin yaroslavafenkin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

…peline/BuildPipelineView/bpp-trigger-pipeline.js

Co-authored-by: Yaroslav <91559310+yaroslavafenkin@users.noreply.github.com>
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.

3 participants