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

chore(ios): removed custom asset image hashing #11437

Merged
merged 3 commits into from Feb 4, 2020

Conversation

vijaysingh-axway
Copy link
Contributor

@build
Copy link
Contributor

build commented Jan 16, 2020

Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 6550 tests are passing.
(There are 701 skipped tests not included in that total)

Generated by 🚫 dangerJS against 6e473b4

Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

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

CR'd the build changes and they look good to me. I have not performed a FR.

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

Native iOS changes are looking good as well!

@ssaddique
Copy link
Contributor

FR Passed.

Test environment:
OS Ver: 10.14.6
Xcode Ver: Xcode 11.3.1
Appc NPM: 4.2.15
Appc CLI: 7.1.2
Ti CLI Ver: 5.2.2
Alloy Ver: 1.14.4
Node Ver: 10.17.0
NPM Ver: 6.13.6
Emulators: iPhone 7 (iOS 10), iPhone X (iOS 11), iPhone XS (iOS 12), iPhone 11 Pro (iOS 13)

@ssaddique ssaddique merged commit 0939b69 into tidev:master Feb 4, 2020
@hansemannn
Copy link
Collaborator

hansemannn commented Feb 5, 2020

This was merged way too early, sorry guys. New regressions:

  • Sub folders are not properly handled anymore (this may be a module issue)
  • Unrelated folders are included in the asset catalog although not inside Resources/iphone (e.g. Resources/test)

@hansemannn
Copy link
Collaborator

This was merged way too early, sorry guys. New regressions:

  • Sub folders are not properly handled anymore
  • Unrelated folders are included in the asset catalog although not inside Resources/iphone (e.g. Resources/test)

@vijaysingh-axway
Copy link
Contributor Author

vijaysingh-axway commented Feb 5, 2020

This was merged way too early, sorry guys. New regressions:

  • Sub folders are not properly handled anymore
  • Unrelated folders are included in the asset catalog although not inside Resources/iphone (e.g. Resources/test)

@hansemannn Can you provide test case please?
In point 2, if you are talking about classic app's folder Resource/test that should go inside Assets.xcassets. Lets say there is an image Resource/test/image.png . In sdk without this change this image will be with hashed value of path inside Assets.xcassets/hashed-path.imageset/Image.png and in SDK with this change it will be inside folder Assets.xcassets/test/Image.imageset/Image.png.

@lokeshchdhry Can you verify with more test cases apart from mentioned in ticket.

@hansemannn
Copy link
Collaborator

@vijaysingh-axway Interesting edge case actually. What if a user wants to use a raw image that shouldn't go to the asset catalog? It's definitely no show stopper, but may get back to the universal-asset-catalog-masterpiece from @cb1kenobi again 😛(happy birthday btw, man!)

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

Successfully merging this pull request may close these issues.

None yet

6 participants