#5139 standard user session plugin#5154
Conversation
There was a problem hiding this comment.
Looks great 👍
I only asked for some documentation (I know you provided a lot, only a few functions that are not too much clear in their role).
- UserSession Plugin should stay in the list of plugins selectable for context.
- moreover, should we enable user session in mapstore by default? What do you think? I think this is valid for georchestra (and it should be set enabled by default also in pluginsConfig.json for that project), but for MapStore, maybe, it should be disabled by default. @tdipisa, what do you think?
web/client/epics/context.js
Outdated
| return message; | ||
| }; | ||
|
|
||
| const flowWithSession = (mapId, contextName, action$, getState) => { |
There was a problem hiding this comment.
I suggest to use createSessionFlow. It looks more clear then flowWithSession and similar to createContextFlow and createMapFlow.
Can you document arguments and returned stream, as for createMapFlow
web/client/epics/context.js
Outdated
| this.name = "context"; | ||
| } | ||
| const createContextFlow = (id, action$, getState) => | ||
| const createContextFlow = (id, session = {}, action$, getState) => |
There was a problem hiding this comment.
Can you document this too, a little
Generally speaking if we have it, we can test it. If we have it, we can use it and better maintain it over time: if this functionlity stays disabled we are never able to identify regressions at this stage. |
I try to translate it in developer terms: That said, no objections to have it enabled by default. Did you consider it also from the final user point of view? Does a user expect to see the last point of view when it reloads the map, instead of resetting to the standard view? |
…nfiguration for the UserSession plugin
|
@offtherailz added the requested docs and pluginsConfig.json configuration for the UserSession plugin |
I need this enabled by default in MS with a client side session storage to properly test it there too. It should be a matter of configuration, isn't it? |
|
@tdipisa I checked, and configuration enables user sessions on localStorage by default |
|
@offtherailz no, this configuration is not allowed at the moment: you mean to enable it only on one specific context or to enable it only for all the contexts? |
User sessions plugin improvements
Description
This work extends user sessions base support, to allow enabling them in the standard product with reasonable defaults, like storing the session on the browser localStorage.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
#5139
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information