Skip to content

Signup landing page for new users - #78

Closed
sumitd94 wants to merge 18 commits into
developfrom
landing-page
Closed

Signup landing page for new users#78
sumitd94 wants to merge 18 commits into
developfrom
landing-page

Conversation

@sumitd94

@sumitd94 sumitd94 commented Apr 9, 2021

Copy link
Copy Markdown
Contributor

This PR fixes issue #70

the signup page can be accessed at /signup?state=get-started

Desktop view

image

Mobile view - iPhone 5/SE

image

iPad

image

Comment thread app/styles/landing.css Outdated
Comment thread app/components/landing.hbs Outdated
Comment thread app/styles/landing.css Outdated

@swarajpure swarajpure left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought we were going to have screens which ask for first name, last name and username after we click on Get Started 🤔
Or is it not so?

@sumitd94

Copy link
Copy Markdown
Contributor Author

I thought we were going to have screens which ask for first name, last name and username after we click on Get Started 🤔
Or is it not so?

Not sure about those, we need to ask @ankushdharkar about that. When the user signs up using GitHub, we will be redirecting them to this page, and when they click on the get-started button, will take them to the signup form.

@swarajpure

Copy link
Copy Markdown
Contributor

I thought we were going to have screens which ask for first name, last name and username after we click on Get Started thinking
Or is it not so?

Not sure about those, we need to ask @ankushdharkar about that. When the user signs up using GitHub, we will be redirecting them to this page, and when they click on the get-started button, will take them to the signup form.

The design link is there in the issue description itself 😅

@akankshadharkar akankshadharkar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a few comments
Also, I think we should change the filename from "form-input" to maybe "signup-form" since the current file doesn't contain just an input but the whole form

Comment thread app/components/sign-up/button.js Outdated
Comment thread app/components/sign-up/button.js Outdated
Comment thread app/components/sign-up/form-input.hbs Outdated
Comment thread app/components/sign-up/button.js Outdated
Comment thread app/components/sign-up/form-input.js Outdated
Comment thread app/components/sign-up/form-input.js Outdated
Comment thread app/components/sign-up/form-input.js Outdated
Comment thread app/components/sign-up/get-started.hbs Outdated
Comment thread app/routes/signup.js
Comment thread app/styles/app.css Outdated

@akankshadharkar akankshadharkar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing the previous comments, Sumit
Left a couple more
Rest lgtm 😄

Comment thread app/components/sign-up/button.hbs
Comment thread app/components/sign-up/button.js Outdated
Comment thread app/components/sign-up/button.js
Comment thread app/components/sign-up/get-started.hbs
Comment thread app/components/sign-up/signup-form.hbs Outdated
Comment thread app/components/sign-up/signup-form.js Outdated
Comment thread app/components/sign-up/signup-form.js
@sumitd94
sumitd94 requested a review from akankshadharkar June 5, 2021 16:00
Comment thread app/components/sign-up/get-started.hbs Outdated
Comment thread app/components/sign-up/get-started.hbs Outdated
Comment thread app/components/sign-up/signup-form.hbs Outdated
Comment thread app/templates/landing.hbs Outdated
Comment thread app/templates/signup.hbs Outdated
Comment thread app/styles/signup.css
Comment thread app/styles/signup.css
Comment thread app/styles/signup.css
Comment thread app/templates/signup.hbs Outdated
Comment thread app/templates/signup.hbs Outdated

@takshch takshch left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Component name sign-up/signup-form is redundant. Please replace it with sign-up/form

@swarajpure swarajpure left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some doubts, please check!

Comment thread app/utils/register-api.js Outdated
Comment thread app/templates/signup.hbs
Comment thread app/templates/signup.hbs
Comment on lines +2 to +11
{{#if (eq this.state "get-started")}}
<SignUp::GetStarted
@changeRouteParams={{this.changeRouteParams}}
@state={{this.state}}
/>
{{else if (eq this.state "thank-you")}}
<SignUp::GetStarted
@changeRouteParams={{this.changeRouteParams}}
@state={{this.state}}
/>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What are these if and else doing differently? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I want to render this component if the value of state is either get-started or thank-you.
But I am not able to find how to use the condition - if (state = 'get-started' || state = 'thank-you') in ember. And so I am rendering like this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're already using ember-truth-helpers in our project
Refering this might help: https://www.npmjs.com/package/ember-truth-helpers/v/2.1.0#usage

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Already tried this. But it's not working.

Comment thread app/controllers/signup.js
Comment on lines +21 to +22
if (paramValue)
this.transitionToRoute({ queryParams: { state: paramValue } });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use router service because this.transitionToRoute is deprecated.
https://deprecations.emberjs.com/v3.x/#toc_routing-transition-methods

@ankushdharkar ankushdharkar added the feature task A big ticket item that needs to come up as a feature label Jul 27, 2021
Comment thread app/routes/signup.js
Comment thread app/routes/signup.js

if (response.status === 401) {
if (response.status == 200 && !userData.incompleteUserDetails) {
alert("You already have filled the up form. You'll now be redirected.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT: single quotes please?

@ankushdharkar ankushdharkar changed the title Landing page for new users Signup landing page for new users Aug 13, 2021
@sumitd94

sumitd94 commented Sep 6, 2021

Copy link
Copy Markdown
Contributor Author

#143 closing this PR because it's messed up so much!

@sumitd94 sumitd94 closed this Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature task A big ticket item that needs to come up as a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants