-
Notifications
You must be signed in to change notification settings - Fork 3
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
Create authenticate feature #31
Create authenticate feature #31
Conversation
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
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'd like to see the events and actions more loosely coupled (cf. what I wrote you in Slack about the add test collection flow): events should indicate something that happened (e.g. CLICKED_LOGIN
), while actions are something that should be done (e.g. setSessionInfo
). Events should be as specific as possible, and actions as abstract as possible. That way, we don't write one action per event, but multiple events can be handled by one action; and also the other way around: multiple actions can handle the same event type depending on the state.
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
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.
Already looks very good!! I'm also a big fan of the finite state machine diagram, definitely something we should keep doing.
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
specifications/pages/technical/set-up-authenticate-feature.adoc
Outdated
Show resolved
Hide resolved
…om:digita-ai/nde-erfgoedinstellingen into docs/set-up-authenticate-feature-675145293
packages/nde-erfgoed-manage/lib/features/authenticate/authenticate.machine.ts
Outdated
Show resolved
Hide resolved
packages/nde-erfgoed-manage/lib/features/authenticate/authenticate.machine.ts
Outdated
Show resolved
Hide resolved
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 good!! I added some stricter type checks, and reworked some of @woutermont 's previous feedback. Look good to me (given that tests will be written in #35 ;-)).
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.
Very nice!
closes #50 |
No description provided.