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

Update mobile brand assets #1927

Merged
merged 4 commits into from
May 13, 2024
Merged

Update mobile brand assets #1927

merged 4 commits into from
May 13, 2024

Conversation

buberdds
Copy link
Contributor

@buberdds buberdds commented May 9, 2024

Copy link

github-actions bot commented May 9, 2024

Deployed to Cloudflare Pages

Latest commit: 5379756f52367bec5a59c535fe23221547fc31a3
Status:✅ Deploy successful!
Preview URL: https://ed66eabb.oasis-wallet.pages.dev

@buberdds buberdds force-pushed the mz/mobile-assets branch 13 times, most recently from b2b9b48 to 24588ad Compare May 9, 2024 12:15
@buberdds buberdds marked this pull request as ready for review May 9, 2024 12:15
@buberdds buberdds requested review from csillag, lukaw3d and lubej May 9, 2024 12:30
Copy link
Collaborator

@lubej lubej left a comment

Choose a reason for hiding this comment

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

Can we commit the changes for resources folder? The folder that assets get generated from. So it's easy to reproduce the current results. + did you minify the original icon/splash?

@lubej
Copy link
Collaborator

lubej commented May 9, 2024

The new design does look way cooler, gg @buberdds on suggesting it 👍

@buberdds
Copy link
Contributor Author

buberdds commented May 10, 2024

Can we commit the changes for resources folder? The folder that assets get generated from. So it's easy to reproduce the current results. + did you minify the original icon/splash?

  1. lol I did not even notice this folder. Why it's not called assets like in Capacitor docs?
  2. I did not minify source files, only generated ones.
  3. hm I don't like idea of adding 9MB of source image files to the repo. These files are controlled and prepared by designers not us

@lubej
Copy link
Collaborator

lubej commented May 10, 2024

  1. lol I did not even notice this folder. Why it's not called assets like in Capacitor docs?

It does support both folders. https://github.com/ionic-team/capacitor-assets#usage. But I think resources was legacy I guess, because before it was community package(from what I remember).

  1. I did not minify source files, only generated ones.

Wait, you went through the trouble to minify every generated one? The last time I did it, I just minified the ones in resources folder.

  1. hm I don't like idea of adding 9MB of source image files to the repo. These files are controlled and prepared by designers not us

But if we nuke android/ios due to an upgrade, or something changes in the structure(like new device aspect ration in any of the platforms). It would be easier, for consistent asset generation. I would just minify them first.

Comment on lines +196 to +199
## Mobile app development

[Capacitor and Ionic docs](docs/mobile-development.md)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Once we release the mobile platform, it would be nice to add extensive guide on how to build, prepare OS for both platforms, etc. 👍

@buberdds
Copy link
Contributor Author

buberdds commented May 10, 2024

Wait, you went through the trouble to minify every generated one? The last time I did it, I just minified the ones in resources folder.

if the trouble is to run one CLI command than yes. Maybe that's why most of prev assets were much larger

But if we nuke android/ios due to an upgrade, or something changes in the structure(like new device aspect ration in any of the platforms). It would be easier, for consistent asset generation. I would just minify them first.

For image manipulation like scaling I guess it's better to use original source file not minified, but Ok I will add assets folder.

@lubej
Copy link
Collaborator

lubej commented May 10, 2024

if the trouble is to run one CLI command than yes. Maybe that's why most of prev assets were much larger

Yes, true, that way it is really simple. Didn't think of that 😅

For image manipulation like scaling I guess it's better to use original source file not minified, but Ok I will add assets folder.

True, but it is usually scaled down from the original image anyway. Don't think there is an image larger than the source file. But anyhow, I agree your way is better 👍 I personally would appreciate if the source files are added to the repo.If you strongly disagree we can leave them out.

@buberdds
Copy link
Contributor Author

hm cannot merge due to stuck Post Run satackey/[email protected]

@buberdds
Copy link
Contributor Author

buberdds commented May 13, 2024

manually wiped out github caches (100+) for this PR as we were running out of avail cache space. will create issue to remove satackey/action-docker-layer-caching as it is deprecated

@buberdds buberdds merged commit fd4cdd8 into master May 13, 2024
13 checks passed
@buberdds buberdds deleted the mz/mobile-assets branch May 13, 2024 06:55
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.

2 participants