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

Create authenticate feature #31

Merged
merged 32 commits into from
Apr 29, 2021

Conversation

lem-onade
Copy link
Collaborator

No description provided.

@lem-onade lem-onade added the documentation Improvements or additions to documentation label Apr 15, 2021
@lem-onade lem-onade self-assigned this Apr 15, 2021
Copy link
Collaborator

@woutermont woutermont left a 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.

@lem-onade lem-onade marked this pull request as ready for review April 22, 2021 13:41
woutermont
woutermont previously approved these changes Apr 22, 2021
@woutermont woutermont mentioned this pull request Apr 22, 2021
Copy link
Collaborator

@wouteraj wouteraj left a 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.

@lem-onade lem-onade enabled auto-merge April 23, 2021 08:56
@lem-onade lem-onade requested a review from wouteraj April 26, 2021 11:20
@lem-onade lem-onade changed the title Set up authenticate feature Create authenticate feature Apr 27, 2021
This was linked to issues Apr 27, 2021
@lem-onade lem-onade disabled auto-merge April 28, 2021 10:14
Copy link
Collaborator

@wouteraj wouteraj left a 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 ;-)).

@wouteraj wouteraj enabled auto-merge (squash) April 29, 2021 05:45
Copy link
Collaborator

@woutermont woutermont left a comment

Choose a reason for hiding this comment

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

Very nice!

@wouteraj wouteraj merged commit 6728fd3 into develop Apr 29, 2021
@wouteraj wouteraj deleted the docs/set-up-authenticate-feature-675145293 branch April 29, 2021 07:32
@wouteraj wouteraj linked an issue Apr 29, 2021 that may be closed by this pull request
2 tasks
@wouteraj
Copy link
Collaborator

closes #50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature:onboarding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase type safety Create login page Set up authenticate feature
3 participants