[7.4-only][LPS-122499, LPS-122448] Remove dom.delegate usages inside frontend-js/* & improve delegate util #531
Conversation
|
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
|
ci:test:relevant |
|
Hey @kresimir-coko, could you please close the other I already see 2 again here which makes it hard to understand which one (if any) is the good one right now. You can always reopen a closed PR, so there's no harm in closing them if you're trying a different approach ;) |
...s/frontend-js-collapse-support-web/src/main/resources/META-INF/resources/CollapseProvider.js
Outdated
Show resolved
Hide resolved
|
@jbalsas alrighty, seems that CI isn't failing here, finally, do we want to freeze this until I add functionality for |
Yes, let's see how this PR looks with that change for comparison. @wincent, do you have any concern with #531 (comment)? |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#6189 |
|
ci:test:relevant |
|
@wincent I've pushed a commit here with a |
Changes looks ok to me. We have automated tests for this file, so maybe we should update those as well to document this feature and ensure it keeps working. |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#3558 |
I've been playing around this for a bit now and I can't figure out how to test if
it('gives access to delegateTarget inside the callback', () => {
document.body.innerHTML = `<div class="match" data-testid="match">
<div>
<div data-testid="most-inner-match"></div>
</div>
</div >`;
const listener = jest.fn();
const event = new Event('click');
event.delegateTarget = getByTestId(
document,
'most-inner-match'
).closest('.match');
delegate(document, 'click', '.match', listener);
userEvent.click(getByTestId(document, 'most-inner-match'));
expect(listener).toHaveBeenCalledWith(event);
});Not sure if I missed the mark by a lot, so a pointer or two would be helpful, I also looked up if we have any tests that contain |
kresimir-coko
force-pushed
the
LPS-122449-frontend
branch
from
975c95d
to
5275280
Compare
|
ci:test:relevant |
|
ci:test:relevant |
kresimir-coko
changed the title
|
@kresimir-coko, as we're getting ready for pushing this forward... would you mind:
|
| @@ -161,4 +161,24 @@ describe('delegate', () => { | |||
|
|
|||
| expect(listener).not.toHaveBeenCalled(); | |||
| }); | |||
|
|
|||
| it('gives access to delegateTarget inside the callback', () => { | |||
There was a problem hiding this comment.
Yea, that was mostly Greg, but he wouldn't have gotten there if I didn't ask him about it!
kresimir-coko
changed the title
|
ci:forward |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
|
Skipping previously passed test suites: |
|
All required test suite(s) passed. |
|
Pull request has been successfully forwarded to brianchandotcom#96241 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#3568 |
Overview
The goal of this PR is two-fold:
delegateutility by expanding it's functionality to includedelegateTargetand optimize it slightly since we're already touching itdom.delegatewith the newdelegateutility inside thefrontend-jsmoduledelegateutilityWhen I started removing the usages of
dom.delegatewe noticed the discrepancy between it and the newdelegateutility. The new one didn't provide access to thedelegateTargetproperty. So we had to improve it before we could properly replace the old one, because otherwise we'd have to change a lot more than just the name of the utility that gets called. Since we did all this work on the utility we decided to improve it as well by optimizing the conditions and hoisting checks higher to prevent expensive calls being invoked if unnecessary.delegateusagesThis is the first PR under this JIRA issue that passed the CI test, so we decided to combine these with the
delegateutility improvements described above so we can instantly see how well they're working together.Testing
The
delegateutility has it's automated tests defined so I used those to test the functionality and added a new test to it with @wincent's help. Outside of that, CI has been proving useful and doing other kinds of tests. I haven't done any manual testing on these files, I tried but couldn't find their usages in a reasonable amount of time.