-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat(ios): Added ios support to existing project #755
Conversation
The part in the brackets should resemble the package you have changed. We also have sort of commit linting wich should have been activated by the Finally please also signoff your commits. this can be done by using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial feedback. If you have the time you can also think about adding iOS support to the separate neon apps (neon_files, neon_news and neon_notes).
We should also evaluate how we can best tackle the notifications support..
Please understand that we are not going to merge this PR until we have a working iOS development setup and gathered some knowledge on how to do iOS flutter development. We are currently also involved in many other issues and short on time so please be patient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong icon.
We currently have a custom script generating them but will probably change it in #447
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should just remove the icons? Where is the script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's currently in tool/generate-assets.sh
.
0a74540
to
70d6864
Compare
@nishp77 Can you make sure that your branch is rebased onto the latest main branch? I see a lot of changes that are already in our main branch (I'm not sure exactly why the show up here). |
65b807d
to
e861850
Compare
@nishp77 do you plan on continuing this? Do you need some help with it? Afaik @Leptopoda has access to a Mac now, so you two could work on it together. |
I think the only thing missing was hooking it up to the icon generation script. Once I have more time I'll test it on some physical iOS devices and test all of our functionality but other than the mentioned notifications issue this should be all. When I checked out the PR I also noticed minor missing things like the package ID being wrong but that should also not be a big problem :) |
Sorry guys got busy because I was away I will starting working back again on this... |
6343f69
to
a7f9b13
Compare
a7f9b13
to
ccc4e7d
Compare
ccc4e7d
to
fb31e39
Compare
fb31e39
to
c39f406
Compare
b44c583
to
182e548
Compare
182e548
to
10e5a30
Compare
@@ -0,0 +1,12 @@ | |||
#!/bin/sh | |||
# This is a generated file; do not edit or check into version control. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
Signed-off-by: Nish <[email protected]> Signed-off-by: Nish Patel <[email protected]> Signed-off-by: Nikolas Rimkis <[email protected]>
I'm going to close this for now and keep the branch locally. |
I have created the issue #754 and this is the PR however please let me know about the commit message I don't know if it's correct or not.