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

Add Ionic Capacitor #1769

Merged
merged 9 commits into from
Jan 30, 2024
Merged

Add Ionic Capacitor #1769

merged 9 commits into from
Jan 30, 2024

Conversation

lubej
Copy link
Collaborator

@lubej lubej commented Nov 22, 2023

Folders android and ios were auto generated with npx cap add [platform]. Android platform - example for Android.

npx cap add android
npx cap add ios

Assets for each platform were generated using @capacitor/assets

@lubej lubej requested a review from lukaw3d November 22, 2023 13:38
@lubej lubej force-pushed the add-ionic branch 2 times, most recently from d065669 to 3f483fe Compare November 22, 2023 13:45
@lubej lubej changed the title Add ionic Add Ionic Capacitor Nov 22, 2023
@lubej lubej force-pushed the add-ionic branch 2 times, most recently from 843104b to 652b279 Compare December 4, 2023 10:03
@lubej lubej requested a review from buberdds December 4, 2023 10:04
@buberdds
Copy link
Contributor

Before adding all these MBs to repo we should run squoosh or some other optimizer.
sample splash png: 2.6MB -> (98% down) 38kB

package.json Show resolved Hide resolved
@@ -0,0 +1,92 @@
@rem
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need files for Win?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wouldn't touch any of autogenerated scripts. Just to spend ages fixing it, if something breaks in the future.


@Test
public void addition_isCorrect() throws Exception {
assertEquals(4, 2 + 2);
Copy link
Contributor

@buberdds buberdds Dec 20, 2023

Choose a reason for hiding this comment

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

good that we have some test coverage here :trollface:

we can remove ExampleUnitTest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would still rather keep this, just in case there is no confusion if we need to generate android/ios platform from scratch, and wonder where did this file came from disappear. But if you are strongly against it, I can delete it as well.

@buberdds
Copy link
Contributor

buberdds commented Dec 20, 2023

Can you provide requirements and steps how these files were generated - sorry I did not look into ionic since weeks and I remember nothing. thx

@lubej
Copy link
Collaborator Author

lubej commented Dec 20, 2023

Can you provide requirements and steps how these files were generated - sorry I did not look into ionic since weeks and I remember nothing. thx

So all files inside android and ios folders are autogenerated.

npx cap add android
npx cap add ios

And I wouldn't change those files, I know there is some useless stuff in it - addressing some comments in the PR.

@lukaw3d
Copy link
Member

lukaw3d commented Dec 20, 2023

What was the reason for including these autogenerated files into the repo again? Is it because of unavoidable modifications we need to make to them later on?

@lubej
Copy link
Collaborator Author

lubej commented Dec 21, 2023

What was the reason for including these autogenerated files into the repo again? Is it because of unavoidable modifications we need to make to them later on?

Yeah. Even though we could in theory just commit diffs, and overwrite generated files on postinstall script. But i am not sure if different version of ionic will generate the same files.

@lubej lubej requested a review from buberdds December 21, 2023 10:11
@lubej lubej force-pushed the add-ionic branch 3 times, most recently from 9265ab3 to ceea95c Compare January 12, 2024 18:18
Copy link

socket-security bot commented Jan 30, 2024

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a59f787) 81.19% compared to head (ceea95c) 81.37%.

❗ Current head ceea95c differs from pull request most recent head b584201. Consider uploading reports for the commit b584201 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1769      +/-   ##
==========================================
+ Coverage   81.19%   81.37%   +0.18%     
==========================================
  Files         192      192              
  Lines        5067     5042      -25     
  Branches      932      923       -9     
==========================================
- Hits         4114     4103      -11     
+ Misses        953      939      -14     
Flag Coverage Δ
cypress 46.05% <ø> (-0.16%) ⬇️
jest 77.16% <ø> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 11 files with indirect coverage changes

@lubej lubej merged commit a6672e2 into oasisprotocol:master Jan 30, 2024
9 of 10 checks passed
@lubej lubej mentioned this pull request Feb 7, 2024
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