Skip to content

[Task-6] As a user, I can view the sign in page - #39

Open
gutakk wants to merge 65 commits into
developfrom
feature/task-6-sign-in-frontend
Open

gutakk wants to merge 65 commits into
developfrom
feature/task-6-sign-in-frontend

Conversation

@gutakk

@gutakk gutakk commented Jun 11, 2021 •

Copy link
Copy Markdown
Owner

Resolve #4

What happened 👀

  • Style the sign in page following the Figma design
  • Add necessary attributes to HTML element
  • Add a necessary component (Alert, AuthLogo and Background)
  • Add necessary env to test workflow
  • Add UI test using Cypress
  • Update README

Insight 📝

Mock the HTTP request in the UI test using cy.intercept() and add the mock response to fixtures

Proof Of Work 📹

@gutakk gutakk added this to the 0.2.0 milestone Jun 11, 2021
@gutakk gutakk self-assigned this Jun 11, 2021
@gutakk gutakk linked an issue Jun 11, 2021 that may be closed by this pull request
@gutakk
gutakk requested a review from rafayet-monon June 14, 2021 11:19
@gutakk
gutakk marked this pull request as ready for review June 14, 2021 11:19
@gutakk
gutakk force-pushed the feature/task-6-sign-in-frontend branch 4 times, most recently from c2ca26a to 0573ad8 Compare June 15, 2021 07:58
@gutakk
gutakk requested a review from carryall June 16, 2021 04:56
Comment thread cypress/integration/sign-in.spec.ts Outdated
Comment thread cypress/integration/sign-in.spec.ts Outdated
Comment thread cypress/integration/sign-in.spec.ts Outdated
Comment thread cypress/support/index.d.ts Outdated
Comment thread src/assets/stylesheets/_variables.scss Outdated
Comment thread src/assets/stylesheets/base/_buttons.scss Outdated
Comment thread src/components/Alert/index.tsx Outdated
Comment thread src/screens/SignIn/index.tsx
Comment thread src/assets/stylesheets/components/_form-input-group.scss Outdated
Comment thread src/assets/stylesheets/components/_alert.scss Outdated
@gutakk
gutakk force-pushed the feature/task-6-sign-in-frontend branch from 0573ad8 to 3152481 Compare June 16, 2021 05:36
@gutakk

gutakk commented Jun 16, 2021

Copy link
Copy Markdown
Owner Author

@rafayet-monon Sorry I didn't notice Cypress convention. I will try to update the code to follow the convention.

@gutakk
gutakk requested a review from rafayet-monon June 16, 2021 09:33
@gutakk
gutakk force-pushed the feature/task-6-sign-in-frontend branch 2 times, most recently from f3da2dd to a7ea289 Compare June 17, 2021 08:44
Comment thread cypress/integration/signIn.spec.ts Outdated
@gutakk
gutakk requested a review from rafayet-monon June 17, 2021 11:24
@gutakk gutakk added the @0.3.0 label Jun 18, 2021
@gutakk gutakk modified the milestones: 0.2.0, 0.3.0 Jun 18, 2021
Base automatically changed from feature/task-5-sign-in-backend to develop June 18, 2021 08:36
@gutakk
gutakk force-pushed the feature/task-6-sign-in-frontend branch from 46d23a2 to 4da5629 Compare June 18, 2021 09:22
Comment thread src/components/AuthLogo/index.tsx Outdated
return (
<div className="auth-logo">
<img className="auth-logo__image" src={logoWhite} alt="Nimble" />
<p className="auth-logo__label">{label}</p>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@gutakk Not sure why do you desire to put this here, IMO this is not quite related to the logo but more like a page title or a form title. So how about moving it out of this component?
Screen Shot 2564-06-18 at 19 05 59

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Because I think all auth page have the same layout that's why I named this component as AuthLogo but maybe we can change to AuthTitle to make it clearer. What do you think?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@gutakk How about page-heder or auth-header, since the title sounds more like the component going to cover only the title? 🤔

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sounds great. Fixed in 1323a67

Comment thread src/assets/stylesheets/components/_auth-logo.scss Outdated
Comment thread src/assets/stylesheets/components/_form-button.scss Outdated
Comment thread src/assets/stylesheets/screens/_sign-in.scss
Comment thread src/assets/stylesheets/components/_form-button.scss
Comment thread src/assets/stylesheets/screens/_sign-in.scss Outdated
Comment thread src/assets/stylesheets/screens/_sign-in.scss Outdated
Comment thread src/assets/stylesheets/base/_forms.scss Outdated
Comment thread .github/workflows/test.yml
Comment thread cypress/integration/SignIn/signIn.spec.ts
Comment thread cypress/integration/SignIn/signIn.spec.ts
@gutakk gutakk added the @0.4.0 label Jun 21, 2021
@gutakk gutakk modified the milestones: 0.3.0, 0.4.0 Jun 21, 2021
@gutakk
gutakk requested a review from carryall June 21, 2021 08:06

@carryall carryall left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@gutakk One last concern seems like the font-weight does nothing. These two labels with font-weight 800 and 400 look exactly the same 😅

<label class="form-input-group__label" for="password">Password</label>
<label class="form-input-group__label" for="password" style="
    font-weight: 400;
">Password</label>

Screen Shot 2564-06-28 at 19 36 36

You might need to convert the font face to web fonts to fix it. Here's the generator that can help you, at least provide woff woff2 you can take a look at the example from The1 project

Comment thread src/components/AuthLogo/index.tsx Outdated
return (
<div className="auth-logo">
<img className="auth-logo__image" src={logoWhite} alt="Nimble" />
<p className="auth-logo__label">{label}</p>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@gutakk How about page-heder or auth-header, since the title sounds more like the component going to cover only the title? 🤔

@gutakk
gutakk requested a review from carryall June 29, 2021 07:03
@gutakk

gutakk commented Jun 30, 2021

Copy link
Copy Markdown
Owner Author

@gutakk One last concern seems like the font-weight does nothing. These two labels with font-weight 800 and 400 look exactly the same 😅

<label class="form-input-group__label" for="password">Password</label>
<label class="form-input-group__label" for="password" style="
    font-weight: 400;
">Password</label>

Screen Shot 2564-06-28 at 19 36 36

You might need to convert the font face to web fonts to fix it. Here's the generator that can help you, at least provide woff woff2 you can take a look at the example from The1 project

Fixed in 810a1d6

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task-6] As a user, I can view the sign in page

3 participants