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

Draft a simple React/Typescript exercise #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JaniAnttonen
Copy link
Contributor

A bit frontend-y, perhaps, but do you have some suggestions?

@JaniAnttonen JaniAnttonen requested a review from vvp November 5, 2021 13:26
Copy link
Contributor

@vvp vvp left a comment

Choose a reason for hiding this comment

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

I like this challenge, not overwhelming if you know what you're doing 👍 Left few comments.


## Requirements

1. Use Typescript
Copy link
Contributor

Choose a reason for hiding this comment

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

And React 18? Should we specify the TS version too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or hmm, did you mean that alternative 1) can be implemented in something else than React?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

React 18 because server components are only available in it. That said, we should specify React as a requirement for the first one as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the first alternative is missing some oomph or interactivity? Should there be at least some state that's being handled?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point and I agree, there should be at least some state management included in this challenge 🤔

What's the most purposeful way then, state changes by reacting to external data changes (polling graphql? etc) or by having user interaction? Including both may bloat the scope of the challenge too much.

Copy link

Choose a reason for hiding this comment

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

How about adding simple "Subscribe to newsletter" form. That should handle quite many use cases for state, handling controlled inputs, loading states, success and error? We could setup up mock api that randomly sends back either 200 OK or some server error?


1. Use Typescript
2. Document setting up the development environment with a README
3. The website should be responsive
Copy link
Contributor

Choose a reason for hiding this comment

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

When talking about requirements, let's be imperative: "Make website responsive."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

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.

3 participants