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

feat: Custom screen options dictionary: Onboarding stack only #1079

Conversation

MosCD3
Copy link
Contributor

@MosCD3 MosCD3 commented Feb 1, 2024

Summary of Changes

Adding more customization to the wallet by specifying a per-screen options dictionary as optional input to the wallet config with default values implemented

Pull Request Checklist

Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this);
  • Updated LICENSE-3RD-PARTY.md for any added dependencies or vendored components;
  • Updated documentation as needed for changed code and new or modified features;
  • Added sufficient tests so that overall code coverage is not reduced.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

Pro Tip 🤓

  • Read our contribution guide at least once; it will save you a few review cycles!
  • Your PR will likely not be reviewed until all the above boxes are checked and all automated tests have passed.

PR template adapted from the Python attrs project.

Signed-off-by: OPS <ops@Mostafas-MacBook-Pro.local>
@MosCD3 MosCD3 requested a review from a team as a code owner February 1, 2024 05:14
Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (04a1fe1) 62.48% compared to head (93fcbe4) 62.64%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1079      +/-   ##
==========================================
+ Coverage   62.48%   62.64%   +0.15%     
==========================================
  Files         172      172              
  Lines        5515     5501      -14     
  Branches     1585     1585              
==========================================
  Hits         3446     3446              
+ Misses       2047     2033      -14     
  Partials       22       22              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@bryce-mcmath bryce-mcmath left a comment

Choose a reason for hiding this comment

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

One issue, also some conflicts to be fixed. Otherwise looks good

packages/legacy/app/ios/Podfile.lock Outdated Show resolved Hide resolved
OPS and others added 4 commits February 15, 2024 11:45
Signed-off-by: OPS <ops@Mostafas-MacBook-Pro.local>
Signed-off-by: OPS <ops@Mostafas-MacBook-Pro.local>
Signed-off-by: OPS <ops@Mostafas-MacBook-Pro.local>
Copy link
Contributor

@bryce-mcmath bryce-mcmath left a comment

Choose a reason for hiding this comment

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

Tiny change requested

Signed-off-by: Mostafa Gamal <46829557+MosCD3@users.noreply.github.com>
@jleach
Copy link
Contributor

jleach commented Mar 11, 2024

@MosCD3 Able to fix the conflicts and we'll re-review. Ty.

Signed-off-by: Mostafa Gamal <moscd3@gmail.com>
Signed-off-by: Mostafa Gamal <moscd3@gmail.com>
Signed-off-by: Mostafa Gamal <moscd3@gmail.com>
Signed-off-by: Mostafa Gamal <moscd3@gmail.com>
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@jleach
Copy link
Contributor

jleach commented Mar 18, 2024

@MosCD3 If this one is still a thing are you able to fix the conflicts. Ty.

Signed-off-by: Mostafa Gamal <moscd3@gmail.com>
Signed-off-by: Mostafa Gamal <moscd3@gmail.com>
Signed-off-by: Mostafa Gamal <moscd3@gmail.com>
@bryce-mcmath
Copy link
Contributor

The entire onboarding stack is now overridable with dependency injection / IoC. Would that be a better approach for this change rather than having config options for each stack and screen as well? @MosCD3 @jleach let me know what you think

@MosCD3
Copy link
Contributor Author

MosCD3 commented Mar 22, 2024

The idea is to allow host wallets to customize Bifold to a pretty much granular level without having to duplicate code. in many cases a host wallet might want to customize title bar background color or font weight without having to create the whole onboarding stack and re-inject it

@bryce-mcmath
Copy link
Contributor

Ok, let's fix the conflicts and we'll get this merged today

@bryce-mcmath bryce-mcmath self-requested a review March 25, 2024 16:39
@bryce-mcmath bryce-mcmath merged commit 0a4903c into openwallet-foundation:main Mar 25, 2024
5 checks passed
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.

None yet

4 participants