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(browser): sign-in session storage #175

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

IceHe
Copy link
Contributor

@IceHe IceHe commented Feb 14, 2022

Summary

feat(browser): sign-in session storage

Related #170

Linear Issue Reference

LOG-1544
LOG-790
LOG-882

Testing

image


@logto-io/eng

@IceHe IceHe force-pushed the icehe-log-1544-sign-in-session-storage branch from b29684b to 5e2856f Compare February 14, 2022 12:16
@IceHe IceHe added the feature Cool stuff label Feb 14, 2022
@IceHe IceHe marked this pull request as ready for review February 14, 2022 12:19
@IceHe IceHe force-pushed the icehe-log-1544-sign-in-session-storage branch from 5e2856f to 3778d39 Compare February 14, 2022 12:46
@IceHe IceHe requested review from gao-sun and charIeszhao February 14, 2022 12:49
Copy link
Contributor

@simeng-li simeng-li left a comment

Choose a reason for hiding this comment

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

nit: wondering whether the signInSession is the only session storage we gonna have.

Just personal preference, instead of defining a signInSession instance within the logtoClient. We could just have to simple sessions util methods sth like

getSessionStorage<T>(key: string,  guard?: SuperstructSchema): T
setSessionStorage(key:string, data: any);

All we need to call getSessionStorage and setSessionStorage wherever it is needed. And util methods can be tested individually.

@IceHe IceHe force-pushed the icehe-log-1544-sign-in-session-storage branch 4 times, most recently from 4a3a5fa to 32fa755 Compare February 14, 2022 17:19
@IceHe IceHe requested a review from simeng-li February 14, 2022 17:20
@IceHe IceHe force-pushed the icehe-log-1544-sign-in-session-storage branch 3 times, most recently from 9e19dbf to 8138cfd Compare February 14, 2022 17:29
@IceHe IceHe force-pushed the icehe-log-1544-sign-in-session-storage branch 4 times, most recently from 05b08b5 to de95f51 Compare February 15, 2022 04:28
@IceHe IceHe requested a review from gao-sun February 15, 2022 04:29
@IceHe IceHe force-pushed the icehe-log-1544-sign-in-session-storage branch 2 times, most recently from ea0a03e to 13761b8 Compare February 15, 2022 06:21
@IceHe IceHe force-pushed the icehe-log-1544-sign-in-session-storage branch from 13761b8 to 029c8c4 Compare February 15, 2022 06:27
Copy link
Contributor

@simeng-li simeng-li left a comment

Choose a reason for hiding this comment

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

overall LGTM

@IceHe IceHe force-pushed the icehe-log-1544-sign-in-session-storage branch from 029c8c4 to d79bfb0 Compare February 15, 2022 08:06
@IceHe IceHe force-pushed the icehe-log-1544-sign-in-session-storage branch from d79bfb0 to 25bd83d Compare February 15, 2022 09:17
@IceHe IceHe requested a review from gao-sun February 15, 2022 09:19
@IceHe IceHe merged commit 98120fd into master Feb 15, 2022
@IceHe IceHe deleted the icehe-log-1544-sign-in-session-storage branch February 15, 2022 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Cool stuff
Development

Successfully merging this pull request may close these issues.

4 participants