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

New method ConfigParser.getByName to find a resource containig a stri… #13

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

Conversation

giuseppelanzi
Copy link

…ng in its src.

Needed to fix cordova-ios, which is erroneously using getBySize when more than one resource have the same dimensions.

Giuseppe Lanzi added 4 commits March 26, 2018 12:36
…ng in its src.

Needed to fix cordova-ios, which is erroneously using getBySize when more than one resource have the same dimensions.
…ng in its src.

Needed to fix cordova-ios, which is erroneously using getBySize when more than one resource have the same dimensions.
@janpio
Copy link
Member

janpio commented Nov 1, 2018

Where and why would this method be useful?

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

We need an answer to the question by @janpio before we consider merging this one:

Where and why would this method be useful?

@brodycj
Copy link
Contributor

brodycj commented Dec 4, 2018

Where and why would this method be useful?

I would favor closing this PR if we do not get this question answered soon. I left a review to avoid a premature merge.

@giuseppelanzi
Copy link
Author

I added it while I was using [email protected] (which I still use a fork of) because getting resource by size is ambiguous: [email protected] has the same size of [email protected].

The [email protected] was missing in my builds because of the mapIconResources method in prepare.js file which defines two resources with same size.

I forked cordova-ios and changed prepare.js removing the resources for apple watch, but it would be more correct to be able to look for a resource by name.
https://github.com/giuseppelanzi/cordova-ios/blob/834cd16d13a5badda80049c2f62a3b60b393c15b/bin/templates/scripts/cordova/lib/prepare.js

The current prepare.js still has the issue
https://github.com/apache/cordova-ios/blob/master/bin/templates/scripts/cordova/lib/prepare.js

Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

Ok, so this is preparatory work for an incoming cordova-ios PR that could fix an issue with it, correct? If so, please make sure that an issue for cordova-ios about this problem you described exists (and links to this PR as a precondition) so we don't forget to actually implement this ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants