-
Notifications
You must be signed in to change notification settings - Fork 23
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
added props and events aligned with latest electron version 1.4.8 #10
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,12 @@ import { Component } from 'react'; | |
|
||
export interface WebViewProps extends React.HTMLAttributes<Electron.WebViewElement>, React.ClassAttributes<WebView> { | ||
src: string | ||
|
||
autosize?: boolean | ||
|
||
autosize?: string; | ||
minwidth?: string; | ||
minheight?: string; | ||
maxwidth?: string; | ||
maxheight?: string; | ||
disablewebsecurity?: boolean | ||
httpreferrer?: string | ||
nodeintegration?: boolean | ||
|
@@ -18,7 +22,7 @@ export interface WebViewProps extends React.HTMLAttributes<Electron.WebViewEleme | |
blinkfeatures?: string | ||
disableblinkfeatures?: string | ||
guestinstance?: string | ||
|
||
onLoadCommit?: EventListener | ||
onDidFinishLoad?: EventListener | ||
onDidFailLoad?: EventListener | ||
|
@@ -28,21 +32,32 @@ export interface WebViewProps extends React.HTMLAttributes<Electron.WebViewEleme | |
onDidGetResponseDetails?: EventListener | ||
onDidGetRedirectRequest?: EventListener | ||
onDomReady?: EventListener | ||
onPageTitleSet?: EventListener | ||
onPageTitleUpdated?: EventListener | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know when this was changed? I'm thinking we leave them both in for compatibility, but with a comment to remove it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They changed this in latest electron api. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, then we should leave them both in for compatibility. I don't think it's worth bumping up our Electron peer dependency just for this. |
||
onPageFaviconUpdated?: EventListener | ||
onEnterHtmlFullScreen?: EventListener | ||
onLeaveHtmlFullScreen?: EventListener | ||
onConsoleMessage?: EventListener | ||
onFoundInPage?: EventListener | ||
onNewWindow?: EventListener | ||
onWillNavigate?: EventListener | ||
onDidNavigate?: EventListener | ||
onDidNavigateInPage?: EventListener | ||
onDestroyed?: EventListener | ||
onMediaStartedPlaying?: EventListener | ||
onMediaPaused?: EventListener | ||
onDidChangeThemeColor?: EventListener | ||
onUpdateTargetUrl?: EventListener | ||
onDevtoolsOpened?: EventListener | ||
onDevtoolsClosed?: EventListener | ||
onDevtoolsFocused?: EventListener | ||
onClose?: EventListener | ||
onIpcMessage?: EventListener | ||
onCrashed?: EventListener | ||
onGpuCrashed?: EventListener | ||
onPluginCrashed?: EventListener | ||
onDestroyed?: EventListener | ||
} | ||
interface WebViewState { | ||
loaded: boolean | ||
webview: Electron.WebViewElement | ||
} | ||
export default class WebView extends Component<WebViewProps, WebViewState> {} | ||
export default class WebView extends Component<WebViewProps, WebViewState> {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,18 +12,29 @@ const EVENTS = [ | |
'did-get-response-details', | ||
'did-get-redirect-request', | ||
'dom-ready', | ||
'page-title-set', | ||
'page-title-updated', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. I'm leaning towards keeping both. |
||
'page-favicon-updated', | ||
'enter-html-full-screen', | ||
'leave-html-full-screen', | ||
'console-message', | ||
'found-in-page', | ||
'new-window', | ||
'will-navigate', | ||
'did-navigate', | ||
'did-navigate-in-page', | ||
'close', | ||
'ipc-message', | ||
'crashed', | ||
'gpu-crashed', | ||
'plugin-crashed', | ||
'destroyed' | ||
'destroyed', | ||
'media-started-playing', | ||
'media-paused', | ||
'did-change-theme-color', | ||
'update-target-url', | ||
'devtools-opened', | ||
'devtools-closed', | ||
'devtools-focused' | ||
]; | ||
|
||
const HANDLERS = EVENTS.map(event => camelCase(`on-${event}`)); | ||
|
@@ -76,7 +87,7 @@ export default class WebView extends React.Component { | |
} | ||
|
||
const tagPropTypes = { | ||
autosize: React.PropTypes.bool, | ||
autosize: React.PropTypes.string, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. Should probably test that this can be a boolean and not be a string. Although if it's too confusing, I'm okay with it being a union type. |
||
disablewebsecurity: React.PropTypes.bool, | ||
httpreferrer: React.PropTypes.string, | ||
nodeintegration: React.PropTypes.bool, | ||
|
@@ -90,6 +101,10 @@ const tagPropTypes = { | |
blinkfeatures: React.PropTypes.string, | ||
disableblinkfeatures: React.PropTypes.string, | ||
guestinstance: React.PropTypes.string, | ||
minwidth: React.PropTypes.string, | ||
minheight: React.PropTypes.string, | ||
maxwidth: React.PropTypes.string, | ||
maxheight: React.PropTypes.string | ||
}; | ||
|
||
const eventPropTypes = EVENTS_HANDLERS.reduce((propTypes, { event, handler }) => { | ||
|
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.
Does this need to be a string? Seems like it is in the Electron docs but I'm pretty sure just the presence of this attribute will trigger it.
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.
On official docs, it says: setting 'on' to enable.
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, but it says that for all of the boolean attributes like
nodeintegration
andplugins
, so I'm fairly certain it behaves like a boolean where the actual value doesn't matter. It just matters if the attribute is there or not.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.
Seems legit