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

Support notifications #36

Merged
merged 24 commits into from
Apr 23, 2021
Merged

Support notifications #36

merged 24 commits into from
Apr 23, 2021

Conversation

wouteraj
Copy link
Collaborator

No description provided.

@wouteraj wouteraj linked an issue Apr 20, 2021 that may be closed by this pull request
@wouteraj
Copy link
Collaborator Author

closes #25

@wouteraj wouteraj marked this pull request as ready for review April 21, 2021 12:34
@wouteraj wouteraj enabled auto-merge (squash) April 21, 2021 12:40
@wouteraj wouteraj self-assigned this Apr 21, 2021
lem-onade
lem-onade previously approved these changes Apr 21, 2021
Copy link
Collaborator

@lem-onade lem-onade left a comment

Choose a reason for hiding this comment

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

added some spaces, lgtm!

packages/nde-erfgoed-manage/lib/app.actions.ts Outdated Show resolved Hide resolved
packages/nde-erfgoed-components/tests/setup.ts Outdated Show resolved Hide resolved
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.

packages/nde-erfgoed-theme/lib/svg.d.ts Show resolved Hide resolved
packages/nde-erfgoed-theme/lib/index.ts Show resolved Hide resolved
packages/nde-erfgoed-manage/lib/app.root.ts Outdated Show resolved Hide resolved
packages/nde-erfgoed-manage/lib/app.root.ts Outdated Show resolved Hide resolved
packages/nde-erfgoed-manage/lib/app.actions.ts Outdated Show resolved Hide resolved
Co-authored-by: Stijn Taelemans <stijn@digita.ai>
@wouteraj
Copy link
Collaborator Author

This might make testing a lot easier and complete:
https://xstate.js.org/docs/guides/testing.html#mocking-effects
https://xstate.js.org/docs/packages/xstate-test/#quick-start
https://timdeschryver.dev/blog/generated-tests-with-xstate-and-cypress

I watched a few videos on @xstate/test an it looks incredible! 🤯

However, I'm not sure if we should adopt it in the scope of this specific pull request. But definitely something to revisit still this week.

@wouteraj
Copy link
Collaborator Author

I noticed that object-curly-spacing is turned off in .eslintrc.json. The comment mentions that it's turned off in favor of TS plugin. Why is that @woutermont ?

"object-curly-spacing": "off", // turned off in favor of TS plugin

@woutermont
Copy link
Collaborator

Those 'in favor of' settings are turned of because another plugin (mostly eslint-typescript) has the exact same setting, of which the documentation prescribes turning the original one off (presumably to prevent clashes)

@wouteraj
Copy link
Collaborator Author

Those 'in favor of' settings are turned of because another plugin (mostly eslint-typescript) has the exact same setting, of which the documentation prescribes turning the original one off (presumably to prevent clashes)

Ok, makes sense. However, it seems that object-curly-spacing is now turned off completely. I'll create a new issue to investigate this.

@woutermont woutermont self-requested a review April 23, 2021 11:09
@wouteraj wouteraj merged commit dc16b49 into develop Apr 23, 2021
@wouteraj wouteraj deleted the feature/notifications branch April 23, 2021 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support notifications
3 participants