-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat(inspectorMode): move inspectorMode outlines on top of the iframe [] #405
Conversation
89953b0
to
4dddebb
Compare
d4588b6
to
9a336cb
Compare
7b9d52d
to
69dee6a
Compare
69dee6a
to
f95a340
Compare
...data, | ||
method, | ||
from: 'live-preview', | ||
source: LIVE_PREVIEW_SDK_SOURCE, | ||
location: window.location.href, | ||
version, | ||
}; | ||
} as MessageFromSDK; |
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.
out of curiosity, why?
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.
Some overlap/confusion inside of typescript due all the different message types.
I will create a story to improve the types for the messages in general as it could produce errors
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.
Okay cool, thanks for explaining!
@@ -5,6 +5,15 @@ import type { SysLink } from 'contentful-management'; | |||
|
|||
import type { LIVE_PREVIEW_EDITOR_SOURCE, LIVE_PREVIEW_SDK_SOURCE } from './constants'; | |||
import { sendMessageToEditor } from './helpers'; | |||
import { |
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.
import type
for any of these?
f95a340
to
cec5b46
Compare
this.handleTaggedElement = this.handleTaggedElement.bind(this); | ||
this.sendAllElements = this.sendAllElements.bind(this); | ||
|
||
// Attach interaction listeners |
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.
nit: maybe it's just me being super picky but I think the naming is a little confusing here as these functions are already "bound", I do understand that it's a different type of binding just as a suggestion maybe something like
this.handleHoverEvents();
this.handleScrollEvents();
this.handleMutationObserverEvents();
this.handleResizeEvents();
then the handlers are bound to the class instance 🤷🏻♂️
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.
or even this.addHoverEventListener...
etc.
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.
you want it, you get it :)
instead of bind
it's now addXYZ
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.
Looks great! ⭐️
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.
💯 nice work!
Move the outlines and interactions of the tagged elements on top of the iframe
Benefit:
Refactored:
Deprecates: