-
Notifications
You must be signed in to change notification settings - Fork 387
Conversation
8e59c84
to
3bb0cd7
Compare
I have a feeling this will need a bit more polish. Curious for feedback on both the general content and the code samples. I'm also curious what we should do with the existing docs around session handling under |
I'd edit the current page to say something like 'you'll need to implement your own class, here is one way to do it'. |
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.
Looks great!
43ae920
to
410dfc0
Compare
A couple updates: Incorporated the suggested changes (thanks for all the input!). I also added some closing remarks to the end of the guide since it ended a bit abruptly, and called out that this is only one of many possible solutions. I'm curious about how the update to the |
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.
I'm curious about how the update to the install dependencies section looks
Looks great to me. It clearly states that it is one possible way of doing it, and provides instructions on how to get started with it.
I would however, consider changing the current notes on sessions
page to link to this one instead of providing its own example, which is too simplistic compared to this one!
docs/index.md
Outdated
@@ -21,5 +21,8 @@ You can follow our getting started guide, which will provide instructions on how | |||
- [Webhooks](usage/webhooks.md) |
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.
Are we updating the getting started guide index accordingly?
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.
I took a look and don't think anything we're doing here effects the getting started guide.
3e12d5e
to
96d2057
Compare
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.
LGTM, but there is one thing that got mentioned on a different issue - let's make it clear that storeCallback
expects to be called multiple times for the same session id, so it should be able to update existing entries.
Good point! I will add a call-out/note about that and then get this merged in. |
6490bee
to
57253ef
Compare
57253ef
to
00bcc43
Compare
00bcc43
to
c194be4
Compare
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.
LGTM!
WHY are these changes introduced?
Fixes #121
WHAT is this pull request doing?
Adds documentation and example code for a
CustomSessionStorage
solution.Type of change
- [ ] Minor: New feature (non-breaking change which adds functionality)- [ ] Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)Checklist
- [ ] I have added/updated tests for this change- [ ] I have documented new APIs/updated the documentation for modified APIs (for public APIs)