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

refactor: Review repository impl and structure #260

Merged
merged 19 commits into from
Nov 23, 2023

Conversation

iFergal
Copy link
Collaborator

@iFergal iFergal commented Nov 17, 2023

Description

This is a general refactor on the repository to clean up the organisation a bit. There are a lot of file changes but most are smaller/import updates.

The core directory only has minor refactoring/doc updates,

  • But enum CredentialType is now in the core as the core should never import from the UI, always vice versa.

Structure outside of ./src:

  • assets is moved into src and README updated
  • icons removed as redundant
  • services directory now contains the cred issuance server and webrtc relay - anything our wallet depends on externally should go here, and other directories relate to building the mobile app.
  • Top-level __mocks__ moved to ./src/ui/__mocks__ which is more appropriate and still works.

UI:

  • Moved contents of ./src/utils into the existing ./src/ui/utils as it makes more sense there.
  • All global constants and types (must be used in multiple places) are now in a globals dir
  • dictionary.ts is now globals/types.ts and only contains types.
    • Introduced a toastMsg Redux value and reducer that displays the current toast message. Before, toasts were of type string and used the operation type (which was of string) - these were mixed up way too much, and TS wasn't helping since everything is a string - we should always use an enum or something here. So we also now have an OperationType enum. This is the biggest change of this PR.

Checklist before requesting a review

Issue ticket number and link

  • This PR has a valid ticket number or issue: [link]

Testing & Validation

  • This PR has been tested/validated in IOS, Android and browser.
  • The code has been tested locally with test coverage match expectations.
  • Added new Unit/Component testing (if relevant). - Not applicable.

Security

  • No secrets are being committed (i.e. credentials, PII)
  • This PR does not have any significant security implications

Code Review

  • There is no unused functionality or blocks of commented out code (otherwise, please explain below)
  • In addition to this PR, all relevant documentation (e.g. Confluence) and architecture diagrams (e.g. Miro) were updated

@iFergal iFergal self-assigned this Nov 17, 2023
Copy link

Vercel PR (merge commit) deploy URL: https://cf-identity-wallet-gbofwrl2a.vercel.app

Copy link

Vercel PR (merge commit) deploy URL: https://cf-identity-wallet-6v0e6g8gp.vercel.app

Copy link

Vercel PR (merge commit) deploy URL: https://cf-identity-wallet-r5j9h2rwz.vercel.app

@iFergal iFergal marked this pull request as ready for review November 20, 2023 16:09
@iFergal iFergal changed the title refactor: Review repository refactor: Review repository impl and structure Nov 20, 2023
sdisalvo-crd
sdisalvo-crd previously approved these changes Nov 21, 2023
Copy link
Contributor

@sdisalvo-crd sdisalvo-crd left a comment

Choose a reason for hiding this comment

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

Looks great, let me know when you have it fully tested 👍

Copy link

Vercel PR (merge commit) deploy URL: https://cf-identity-wallet-2x5sr00yx.vercel.app

jimcase
jimcase previously approved these changes Nov 22, 2023
Copy link

Vercel PR (merge commit) deploy URL: https://cf-identity-wallet-1rk8sf25c.vercel.app

jimcase
jimcase previously approved these changes Nov 23, 2023
sdisalvo-crd
sdisalvo-crd previously approved these changes Nov 23, 2023
@iFergal iFergal dismissed stale reviews from sdisalvo-crd and jimcase via 61ecf9e November 23, 2023 11:50
Copy link

Vercel PR (merge commit) deploy URL: https://cf-identity-wallet-8ndhws2sg.vercel.app

@iFergal iFergal merged commit d9cf5f2 into develop Nov 23, 2023
1 check passed
@iFergal iFergal deleted the refactor/DTIS-452-Review-Repository branch November 23, 2023 12:14
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