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

As a guest, I can register #23

Merged
merged 33 commits into from
Jan 28, 2021
Merged

Conversation

hoangmirs
Copy link
Owner

@hoangmirs hoangmirs commented Dec 17, 2020

Solve #2

What happened 👀

Insight 📝

  • Add registration feature, show flash messages when failed or succeeded
  • Add /register route
  • Create BaseController to add some common works that we re-use in several controllers
  • Add some helpers to encrypt password, set some common controller attributes, convert string
  • Split the tailwind's components into multiple files to resolve the slow-building bug when using Tailwind 2 + Webpack. Will merge them again when Tailwind fixed the issue
  • Add copy-webpack-plugin to copy the static assets to static folder

Proof Of Work 📹

Create successfully
2021-01-06 10 30 59

Create failed
2021-01-06 10 32 08

Copy link
Collaborator

@olivierobert olivierobert left a comment

Choose a reason for hiding this comment

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

Early feedback

views/shared/header.html Outdated Show resolved Hide resolved
patterns: [
{
from: path.resolve(__dirname, "../assets/images"),
to: path.resolve(__dirname, "../static/images"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

We need to set the static path for the assets/images folder if we need to use it. But I want to put all static files in static folder, then I go with this, is it okay?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure of the motivation to move images to static/ 🤔 Do you mean that once the build is completed, the compiled JS and CSS would go to /static to you prefer to have images there too?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No, before I couldn't use assets from the assets folder, I have to set the static path to use it, then I went with putting all the static files in the same folder.
But I changed to use assets folder in 45a4a02

Base automatically changed from chore/setup-ci-cd to development December 18, 2020 08:13
Copy link
Collaborator

@olivierobert olivierobert left a comment

Choose a reason for hiding this comment

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

Some early feedback again

assets/stylesheets/layouts/authentication.scss Outdated Show resolved Hide resolved
assets/stylesheets/app-components.scss Show resolved Hide resolved
bootstrap/db.go Show resolved Hide resolved
helpers/to_snake_case.go Outdated Show resolved Hide resolved
controllers/registration.go Outdated Show resolved Hide resolved
controllers/registration.go Outdated Show resolved Hide resolved
@hoangmirs hoangmirs self-assigned this Jan 6, 2021
@hoangmirs hoangmirs added this to In progress in Version 1.0.0 via automation Jan 6, 2021
@hoangmirs hoangmirs added this to the 1.0.0 milestone Jan 6, 2021
@hoangmirs hoangmirs linked an issue Jan 6, 2021 that may be closed by this pull request
7 tasks
@hoangmirs hoangmirs changed the title [WIP] As a guest, I can register As a guest, I can register Jan 6, 2021
@hoangmirs hoangmirs marked this pull request as ready for review January 6, 2021 04:11
@hoangmirs hoangmirs requested a review from Lahphim January 6, 2021 04:12
Copy link
Collaborator

@gutakk gutakk left a comment

Choose a reason for hiding this comment

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

Please add the minimum node version can be used in this project as I cannot use version 10 with this project.

controllers/base.go Show resolved Hide resolved
controllers/base.go Outdated Show resolved Hide resolved
controllers/base.go Show resolved Hide resolved
c.Layout = "layouts/authentication.html"
c.TplName = "registration/new.html"

c.Data["Title"] = "Create a new account"
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about move the Create a new account to const variable as I saw you use this more than one places and also the Layout and TplName

Copy link
Owner Author

Choose a reason for hiding this comment

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

I moved them into a new function

forms/registration.go Outdated Show resolved Hide resolved
func (registrationForm *RegistrationForm) Valid(v *validation.Validation) {
if registrationForm.Password != registrationForm.PasswordConfirmation {
// Set error messages of Name by SetError and HasErrors will return true
err := v.SetError("Password", "Password confirmation does not match")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about this error message?

Password does not match password confirmation

It a little bit confuse what password confirmation does not match with.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I changed. Actually, we can not say Password confirmation does not match the email or something else, but to avoid your confusion, it should be changed


success, err := valid.Valid(registrationForm)
if err != nil {
logs.Error("Validation error:", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this what you expect. When I tried to register with passwords don't match, this printed out to the console

SetError error: Password confirmation does not match

Nothing about Validation error:

Copy link
Owner Author

Choose a reason for hiding this comment

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

This just logs the error when Valid() doesn't work as expected, I don't know when it will raise that error, but the Valid() function returns a success value and an error object, so we have to handle the error

main.go Show resolved Hide resolved
<div class="navigation-menu">
<div class="navigation-menu__left">
<a class="navigation-menu__logo" href="/">
<img src="/static/images/logo.svg" alt="Go Scraper" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about use background-image for this. You can find more information here. cc @olivierobert

Copy link
Owner Author

Choose a reason for hiding this comment

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

I changed to use the background-image, will research about the SVG inline later

@gutakk
Copy link
Collaborator

gutakk commented Jan 19, 2021

Screen Shot 2564-01-19 at 15 52 18
This error message is not user friendly after I tried to register with duplicate email.

Version 1.0.0 automation moved this from Review in progress to Reviewer approved Jan 20, 2021
@hoangmirs hoangmirs merged commit 95d1b82 into development Jan 28, 2021
Version 1.0.0 automation moved this from Reviewer approved to Done Jan 28, 2021
@hoangmirs hoangmirs deleted the feature/registration-page branch January 28, 2021 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

As a guest, I can register
4 participants