Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Electron4 changes #669

Closed
wants to merge 5 commits into from
Closed

Electron4 changes #669

wants to merge 5 commits into from

Conversation

joshuef
Copy link
Collaborator

@joshuef joshuef commented Apr 10, 2019

Fixes #289

Also removes react-icons #492 #455

QA:

  • Regression checks.
  • Check add/remove buttons exist (they will have new style from antd).

@joshuef joshuef force-pushed the Electron4Changes branch 3 times, most recently from c3b560e to dff7caa Compare April 10, 2019 13:28
@hunterlester
Copy link
Contributor

hunterlester commented Apr 10, 2019 •
edited
Loading

Looking into why notifications are not showing for blocked web requests.
Seems that did-fail-load event is not firing.

@joshuef joshuef marked this pull request as ready for review April 11, 2019 08:24
@joshuef joshuef force-pushed the Electron4Changes branch 5 times, most recently from adb8c4f to ae2037b Compare April 11, 2019 10:51
@joshuef
Copy link
Collaborator Author

joshuef commented Apr 11, 2019

@hunterlester FYI buffer stuff removed from this PR as was wreaking unexpected havoc w/ CI

@hunterlester
Copy link
Contributor

@joshuef Everything appears to be working nicely, except for the did-fail-load event still not firing on webview when a web request is blocked.
I'm using this branch and your Bviews branch to rebase my 651, BrowserView draft, where a blocked web request is firing a did-fail-load on BrowserView webContents.

@joshuef
Copy link
Collaborator Author

joshuef commented Apr 15, 2019

@hunterlester just to be clear, this is working in master, correct?

@hunterlester
Copy link
Contributor

hunterlester commented Apr 15, 2019 •
edited
Loading

@joshuef Correct, working in master, as in did-fail-load event is triggered when web request blocked, leading to notification.

@joshuef joshuef mentioned this pull request Apr 22, 2019
Closed
@joshuef joshuef force-pushed the Electron4Changes branch 2 times, most recently from 42947f3 to fd7221d Compare April 22, 2019 11:01
@joshuef
Copy link
Collaborator Author

joshuef commented Apr 22, 2019

Hmmmm ... I'm unable to find if triggering did-fail-load is something that is 'supposed' to happen with blocked reqs in electron.

Which raises a Q:

Do we simply need to trigger the notification elsewhere for this PR to be valid?

Or do we shelve electron 4 upgrades until browserview, which behaves as master it sounds like (and also implies that maybe did-fail-load should be triggered)

@joshuef
Copy link
Collaborator Author

joshuef commented Apr 22, 2019

Also: i'm noticing that the shortcut to select the address bar is not working atm.

@joshuef joshuef closed this Apr 26, 2019
@joshuef joshuef deleted the Electron4Changes branch September 6, 2019 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hunterlester hunterlester hunterlester left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade Electron.
2 participants
@joshuef @hunterlester