Add remove method #54
Conversation
There was a problem hiding this comment.
I think you're right that it makes sense to have a method which removes the floating focus effect!
See my comments for nitpicks on how I'd personally like to see it implemented, and I'll ask my colleague for a quick second opinion (because I haven't been doing too much frontend stuff lately).
src/floating-focus.js
Outdated
| @@ -226,4 +235,9 @@ export default class FloatingFocus { | |||
| repositionElement(target, floater) { | |||
| Object.assign(floater.style, this.getFloaterPosition(target)); | |||
| } | |||
|
|
|||
| remove(){ | |||
There was a problem hiding this comment.
We activate the floating focus effect by instantiating this with its constructor, so therefore I find it a bit strange to have a remove() method like this which then leaves the class sort of floating in space.
How about renaming this to deactivate() or disable() or something similar, and then also implementing an activate() or enable() method which can enable it again? (In the end that would only call addEventListeners again but it makes it clearer from the outside what you're doing)
There was a problem hiding this comment.
I think you are right, it is clearer and more personalized.
Co-authored-by: Jirinrin <[email protected]>
Co-authored-by: Jirinrin <[email protected]>
Add a method to remove the floating focus to be able to implement customizations in a simple way