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

refact: enhance state management and full typescript support #15

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

aminbenmansour
Copy link

cleaner and more readable code

Copy link
Contributor

@AlexAndrei98 AlexAndrei98 left a comment

Choose a reason for hiding this comment

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

Thank you for cleaning up the code and actually make it fully typescript! ! It is much better organized!

salty: Salty;
}

interface Salty {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these types can also be added to the native signify-ts client rather than in the web app itself! It would be awesome if you could make a pr here as well to add these!

Copy link
Author

Choose a reason for hiding this comment

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

Sure! Thank you for pointing out to this!

@aminbenmansour
Copy link
Author

I thought it would be more understandable for newcomers and much easier and less intimidating to add new features if the code was strongly typed and split into a hierarchy :)

@2byrds
Copy link
Contributor

2byrds commented Aug 29, 2023

I plan to review/test this tonight or tomorrow

All variables in shared-slice are related to authentication, thus, auth-slice is more semantic. We can have a shared-slice later if needed
@2byrds
Copy link
Contributor

2byrds commented Aug 30, 2023

I had some issues running the orig repo (and yours) but they are on my side. I'll come back to this soon.

@aminbenmansour
Copy link
Author

please reinstall dependencies using yarn

@AlexAndrei98
Copy link
Contributor

@2byrds have you had a chance to integration test this PR?

@2byrds
Copy link
Contributor

2byrds commented Sep 5, 2023

No, I've been buried in other tasks. I'll try to revisit soon, thank you for the reminder!

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