-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
Better document lib.hudson.widget-refresh
#10237
Better document lib.hudson.widget-refresh
#10237
Conversation
Behaviour.specify( | ||
".widget-refresh-reference", | ||
"widget-refresh", | ||
0, | ||
function (e) { | ||
var id = e.getAttribute("data-id"); | ||
var url = e.getAttribute("data-url"); | ||
let id = e.getAttribute("data-id"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from var to let improves code safety
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that extra change would suit the scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes from var
to let
already, so I'm interpreting this comment as a confirmation of the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see! Thanks for clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-beck please, resolve comments in this PR.
@A1exKH Please clarify which comments need resolving. |
There seem to be some issues need to be resolved in this PR. For example, there are 2 linting bugs reported at https://github.com/jenkinsci/jenkins/pull/10237/checks?check_run_id=36656792200. |
Thanks, I missed that. Seems weird that IntelliJ is not picking up on these declarations and/or that we do not allow this to compensate for poor IDE behavior, but especially the latter seems like a topic for another time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-beck LGTM.
/label ready-for-merge This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. |
This is also used outside core, making it essentially API, so deserves to be documented. While I'm here, address some IDE complaints about my JS.
Testing done
None
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist