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.

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

CB-12468 added ability to set|get default options; #213

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

IvanProdaiko94
Copy link

@IvanProdaiko94 IvanProdaiko94 commented Feb 17, 2017

While using plugin I must set popup features only when calling window.open method. But a lot of libraries (firebase, openFB) use window.open inside their own code where I can't directly menage the features of window to be opened, and on IOS, where we do not a have a 'back' button we could have a problem. For example window, opened with 'toolbar=no' will make app to stack with no ability to close the window. Another thing to be mentioned is when on IOS you use

So, my proposal is to provide an ability to setDefaultOptions method that will help to prevent external libraries to manage the options of of window to be opened directly. If pass a '(required)' in option like that 'location=no(required)' than any option provided in window.open method will not affect on the default settings.

I tested it in my projects (IOS|Android), and everything works fine;

@cordova-qa
Copy link

Cordova CI Build has one or more failures.

Commit - Link
Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

@IvanProdaiko94 IvanProdaiko94 changed the title CB-12365 added ability to set|get default options; CB-12468 added ability to set|get default options; Feb 17, 2017
@cordova-qa
Copy link

Cordova CI Build has completed successfully.

Commit - Link
Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

@cordova-qa
Copy link

Cordova CI Build has one or more failures.

Commit - Link
Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

@cordova-qa
Copy link

Cordova CI Build has completed successfully.

Commit - Link
Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

@oleksmir
Copy link

Any estimates to merge it in master?

@cordova-qa
Copy link

Cordova CI Build has one or more failures.

Commit - Link
Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

@alsorokin
Copy link
Contributor

Let there be tests

@cordova-qa
Copy link

Cordova CI Build has one or more failures.

Commit - Link
Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

@cordova-qa
Copy link

Cordova CI Build has completed successfully.

Commit - Link
Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

@IvanProdaiko94
Copy link
Author

Any changes to be rebased to master? What shell I do to improve pull in order to get this done?

@olgakozoriz
Copy link

Have the same issue on IOS. Probably fixing ios is needed to, but seems that it's good workaround compatible with all systems. Why not merge?

Copy link

@sd5555555 sd5555555 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems logical, why not merge?

@infil00p
Copy link
Member

We don't merge if it doesn't merge cleanly, that's why it didn't get merged most likely.

@nosovk
Copy link

nosovk commented Jan 23, 2018

It was ok year ago :)
@IvanProdaiko94 could you please rebase it.

@IvanProdaiko94
Copy link
Author

Give this PR one more try, please)

@nosovk
Copy link

nosovk commented Mar 24, 2018

Some CI check seems to be broken :(

@janpio janpio added this to ⛔ Blocked: Tests failing in Apache Cordova: Plugin Pull Requests Sep 16, 2018
@janpio
Copy link
Member

janpio commented Sep 16, 2018

@IvanProdaiko94 could you please rebase onto master once more? Maybe this will reset and fix the tests. Thanks.

@janpio janpio moved this from ⛔ Blocked: Tests failing to ⏳ Ready for Review in Apache Cordova: Plugin Pull Requests Sep 16, 2018
@IvanProdaiko94
Copy link
Author

@janpio ok

styling fixes

final eslint fix

fixed types
@janpio janpio moved this from ⏳ Ready for Review to ⛔ Blocked: Tests failing in Apache Cordova: Plugin Pull Requests Oct 1, 2018
@janpio
Copy link
Member

janpio commented Oct 2, 2018

One last rebase on master should get the tests passing now.

@janpio
Copy link
Member

janpio commented Oct 3, 2018

Closing and re-opening to trigger a new CI/test run with new PR merge.

@janpio janpio closed this Oct 3, 2018
Apache Cordova: Plugin Pull Requests automation moved this from ⛔ Blocked: Tests failing to ☠️ Closed/Abandoned Oct 3, 2018
@janpio janpio reopened this Oct 3, 2018
Apache Cordova: Plugin Pull Requests automation moved this from ☠️ Closed/Abandoned to 🐣 New PR / Untriaged Oct 3, 2018
@janpio janpio moved this from 🐣 New PR / Untriaged to ⏳ Ready for Review in Apache Cordova: Plugin Pull Requests Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet