-
Notifications
You must be signed in to change notification settings - Fork 2
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: setup UserStore
and basic visual feedback throughout editor based on access
#2214
Conversation
Removed vultr server and associated DNS entries |
UserStore
and basic visual feedback throughout editor based on access
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.
A few comments on PR, no show stoppers! Looking great and feels very solid 👏 I'm sure Alastair and Ian will have some design thoughts but that's 100% a separate issue.
A few unit tests for maybe some components and UserStore methods would probably be wise as a next step 👍
Hitting a few issues small on the Pizza mainly related to the teamStore not being populated. I think this is related to an issue we've discussed before around how teams are handled within the editor (window.api.getState().getTeam()
is perfect in /preview
and /unpublished
but inconsistent in the Editor as you've flagged in this PR.
I think we'll need to resolve this before going for a prod deploy - I'll queue on up just now so we can get the code changes from already merged to main
pushed forward in the meantime.
editor.planx.uk/src/pages/FlowEditor/components/Flow/components/Checklist.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/pages/FlowEditor/components/Flow/components/Question.tsx
Outdated
Show resolved
Hide resolved
Just realised in the demo...
I'm not exactly sure when the "update" happens - I think it's on save of the modal but juuuuust in case? And I guess it's about expected behaviour also. |
@@ -365,7 +365,6 @@ export const editorStore: StateCreator< | |||
toBefore, | |||
})(get().flow); | |||
send(ops); | |||
get().resetPreview(); |
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.
so this is a notable change! moveNode
is the only graph method that called this shared store method, which causes the window to reload, the user to no longer be set, and therefore the flow incorrectly reloads as if it's now view-only rather than edit-access as intended.
from what I can tell, resetPreview
is intended to be called outside the Editor based on the variables it's setting, and in testing I can't see any in-editor issues with removing it (the operation is still successful, the flow order after move persists as expected, etc). But please do test this!
Changes
UserStore
with simple state management getters & setters for working with users & their roles within all authenticated routesVisual feedback examples
Teams
Team
Flow
<Hanger />
points in the graph which makes it impossible to move, add or clone nodes (in the UI at least, not the actualoperations
table!)Node
** I learned our original hypothesis about not connecting to ShareDB altogether, rather than restricting per operation method, doesn't actually work very well here because the
connect
&connectTo
Editor store methods are actually responsible for loading the whole flow - so if we don't proceed with the connection, it just shows an empty graph!Open questions / future improvements
TeamStore
be initiated ideally? For now, I often rely onwindow.location.pathname
to get theteamSlug
instead (which works totally fine, but doesn't feel the cleanest and assumes the editor will never be hosted at a custom subdomain!)