-
Notifications
You must be signed in to change notification settings - Fork 17
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
Use custom events for SHED #1680
base: georgie
Are you sure you want to change the base?
Use custom events for SHED #1680
Conversation
…tentional, and does not need to be 'fixed'
@ricksbrown This ready to review. |
@@ -38,16 +38,6 @@ define(["wc/has", "wc/dom/classList", "wc/timers"], function(has, classList, tim | |||
} | |||
} | |||
|
|||
if (has("ie") === 8) { |
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 is the right spirit however I suggest we delete the entire JS file (and remove any calls to it elsewhere).
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 concur
shed.subscribe(shed.actions.COLLAPSE, handleCollapseOrHide); | ||
shed.subscribe(shed.actions.SHOW, instance.onshow); | ||
shed.subscribe(shed.actions.HIDE, handleCollapseOrHide); | ||
event.add(document.body, shed.events.EXPAND, instance.onexpandlistener); |
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.
We aim for camelCase close to 100%. Not sure why it wasn't adhered to before and I can see you have simply carried that over. Unless I am missing something I suggest we make it camelCase now.
…components into use-shed-events
* | ||
* @param {Event} $event The shed event that fired. | ||
*/ | ||
this.onshowlistener = function($event) { |
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.
Why is this public?
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.
also camelCase the function name
* | ||
* @param {Event} $event The shed event that fired. | ||
*/ | ||
this.onexpandlistener = function($event) { |
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.
And this one, does it need to be public?
*/ | ||
function shedAjaxSubscriber(element, action) { | ||
function shedAjaxSubscriber($event) { | ||
var element = $event.target, |
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.
Pretty sure this breaks shedAjaxSubscriber
when it is called via processResponse.subscribe
- looks like the introduction of a bug to me.
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 is definitely a problem - but why didn't our testing pick it up (well the answer to that is obvious and so I need to go write all those tests I have been promising since we moved off Doh) ?
}, { priority: Observer.priority.HIGH }); | ||
observer.subscribe(function() { | ||
order.push("high"); | ||
return Promise.reject("Observer testing reject in high subscriber"); | ||
return Promise.reject("Intentional error: Observer testing reject in high subscriber"); |
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.
Yeah this is good 👍
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.
Pretty sure there's a regression in there...
FYI I suggest when we merge this we squash rather than regular merge.
@JohnMcGuinness you have put a lot of effort into this and it is mostly good. Can you fix the few issues identified by @ricksbrown and resubmit the PR please. |
Updated multiSelectPair to use custom shed events