refactor: remove jq dependency #35
Conversation
tcarac
commented
Feb 24, 2024
tcarac
commented
- Add Fallback strategy
- Remove unnecessary registry
- Remove Jquery dep
- Upgrade node
tcarac
force-pushed
the
tcarac/cleanup
branch
2 times, most recently
from
49ddaee
to
87c5717
Compare
tcarac
force-pushed
the
tcarac/cleanup
branch
from
87c5717
to
3827213
Compare
| return pluginFn() || {}; | ||
| } | ||
| export function hover(pluginFn, el) { | ||
| const isJqueryEl = el.jquery !== undefined; |
There was a problem hiding this comment.
Can we just leave it up to the the client code to add a new hover strategy with widget.strategies.add('hover', () => {}), if they want to have a jQuery hover option?
There was a problem hiding this comment.
In fact I don't think this works, since #31 the hover strategy has been broken.
Notice that in UE we're using 1.3.0. We use 2.0.0 in another project and haven't used the hover strategy in that project yet.
There was a problem hiding this comment.
Got it, still keeping the registry and opening it up to client registering new strategies makes sense.
| el.addEventListener('mouseover', pluginFn, { once: true }); | ||
| } | ||
| export function fallback(strategy, fallback) { | ||
| console.warn(`Strategy ${strategy} not found, falling back to ${fallback.name} strategy.`); |
There was a problem hiding this comment.
Maybe just throw InitiationNotSupportedError? This is a major mistake, it doesn't feel like a warning is sufficient.
There was a problem hiding this comment.
Yeah, that's what I prefer anyway. I don't think throwing a "function undefined" method is helpful, but having a strict interface is good too.
There was a problem hiding this comment.
Actually I'm wrong, the fallback with warning is a good idea.
Moving a widget into a different environment probably shouldn't break it.
There was a problem hiding this comment.
Actually I'm wrong, the fallback with warning is a good idea.
Moving a widget into a different environment probably shouldn't break it.
Yeah, that was my line too 👍
|
Accidental merge here, going to revert. The problem is that we are actively using the registry ourselves, we can't get rid of it. |
