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

Add boilerplate code #1

Merged
merged 1 commit into from
Jul 9, 2020
Merged

Add boilerplate code #1

merged 1 commit into from
Jul 9, 2020

Conversation

Joozty
Copy link
Member

@Joozty Joozty commented Jun 18, 2020

@viktor-yakubiv Consider this just basic boilerplate. I created templates for all three steps. We should probably merge it and start building registration on top of it. I still haven't figured out global store management. I think we don't need it for now. So it should be decided and added later.

@vercel
Copy link

vercel bot commented Jun 18, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oacore/search/5k4425pve
✅ Preview: https://search-git-first-step.oacore.vercel.app

@Joozty Joozty changed the title WIP: Add first step WIP: Add boilerplate code Jun 24, 2020
@Joozty Joozty self-assigned this Jun 24, 2020
@Joozty Joozty changed the title WIP: Add boilerplate code Add boilerplate code Jul 2, 2020
@Joozty Joozty requested a review from viktor-yakubiv July 2, 2020 09:37
@Joozty Joozty marked this pull request as ready for review July 2, 2020 09:37
Copy link
Member

@viktor-yakubiv viktor-yakubiv left a comment

Choose a reason for hiding this comment

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

I see you copied most of the code. I do not very like it because it takes a lot of mess with it. Although, this is a better approach than creating the same mess from scratch. Let's refactor it step-by-step, I hope we will have time for it 👍

Some of my comments below describe the desired refactoring. They may be discussed but I expect them to be resolved with no action but another task is created instead ☝️

The code itself is hard to understand in some places, maybe because you were missing the context (it's my fault, sorry). Could you please simplify the code taking the following actions and ask for the review once again?

  1. We do not need the second page ❌
  2. The third page may be needed but later, but much later, you can either remove or store it somewhere. I propose to copy the branch and store it on GitHub but remove it from here ⚠️
  3. It would be nice to add a card around the form(s) but it can be done separately ✅
  4. I personally would like to merge templates back with the pages but it minor too ✅

next.config.js Show resolved Hide resolved
next.config.js Show resolved Hide resolved
next.config.js Show resolved Hide resolved
pages/_app/hooks.js Show resolved Hide resolved
utils/debounce.js Show resolved Hide resolved
@Joozty
Copy link
Member Author

Joozty commented Jul 6, 2020

@viktor-yakubiv Thanks for your review. Please have a look now.

Copy link
Member

@viktor-yakubiv viktor-yakubiv left a comment

Choose a reason for hiding this comment

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

Thank you for removing extra pages. With your help, I managed to find which page exactly I should review. Please, remove a bit more extra code and I consider it's ready.

I created tasks for the conversations needed to resolve separately.

templates/data-provider/index.jsx Outdated Show resolved Hide resolved
templates/data-provider/index.jsx Outdated Show resolved Hide resolved
templates/data-provider/index.jsx Outdated Show resolved Hide resolved
pages/_app/hooks.js Show resolved Hide resolved
templates/data-provider/index.jsx Show resolved Hide resolved
Copy link
Member

@viktor-yakubiv viktor-yakubiv left a comment

Choose a reason for hiding this comment

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

It's an awesome start 👍

@Joozty Joozty merged commit 9202b89 into master Jul 9, 2020
@Joozty Joozty deleted the first-step branch July 9, 2020 07:43
viktor-yakubiv pushed a commit that referenced this pull request Jul 16, 2020
Project structure is cloned from the github.com/oacore/dashboard
but is simplified and adjusted to the current needs.
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.

2 participants