-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: design registration form pop up #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use the dir structure and messages names as in https://github.com/edx/frontend-component-authn-edx/pull/4/files? this is for consistency
04fbb67
to
5661a59
Compare
9c707df
to
d913ac1
Compare
0a6ce38
to
33c277c
Compare
33c277c
to
7f21f8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there were some changes that Ahtesham missed in his PR that he wanted you to do as part of this ticket.
src/forms/register/index.jsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the file name to registration-popup
src/forms/register/index.jsx
Outdated
import SocialAuthButtons from '../../common-ui/SocialAuthButtons'; | ||
import './index.scss'; | ||
|
||
const RegisterForm = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RegisterForm = () => { | |
const RegistrationForm = () => { |
src/authn-component/index.jsx
Outdated
<BaseContainer open={open} setOpen={setOpen}> | ||
<LoginForm /> | ||
</BaseContainer> | ||
<BaseContainer open={open} setOpen={setOpen} footerText={registrationFooterText}> | ||
<RegisterForm /> | ||
</BaseContainer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we decided to use a single BaseContainer in the standup.
src/authn-component/messages.jsx
Outdated
footerText: { | ||
id: 'footer.text', | ||
defaultMessage: 'By creating an account, you agree to the Terms of Service and Honor Code and you acknowledge that edX and each Member process your personal data in accordance with the Privacy Policy.', | ||
description: 'Text for footer', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: 'Text for footer', | |
description: 'Text that appears on registration form stating honor code and privacy policy', |
src/forms/register/index.jsx
Outdated
<Form.Group controlId="email" className="w-100 mb-0"> | ||
<Form.Control | ||
type="email" | ||
floatingLabel={<span>{formatMessage(messages.registerFormEmailFieldLabel)}</span>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need span around it?
src/forms/register/messages.jsx
Outdated
registerFormContinueButton: { | ||
id: 'register.form.continue.button', | ||
defaultMessage: 'Continue', | ||
description: 'Text for submit button', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
src/forms/register/messages.jsx
Outdated
registerFormAlreadyHaveAccountText: { | ||
id: 'register.form.already.have.account.text', | ||
defaultMessage: 'Already have an account?', | ||
description: 'Text for already have an account', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: 'Text for already have an account', | |
description: 'Text for registration button', |
src/forms/register/messages.jsx
Outdated
registerFormSignInLink: { | ||
id: 'register.form.sign.in.link', | ||
defaultMessage: 'Sign In', | ||
description: 'Text for signing in', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: 'Text for signing in', | |
description: 'Text for sign in link', |
src/forms/register/messages.jsx
Outdated
registerFormAccountSchoolOrganizationText: { | ||
id: 'register.form.account.school.organization.text', | ||
defaultMessage: 'Have an account through school or organization?', | ||
description: 'Text for having an account through school or organization', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: 'Text for having an account through school or organization', | |
description: 'Label for link that leads learners to the institution login page', |
src/forms/register/messages.jsx
Outdated
defaultMessage: 'Sign In', | ||
description: 'Text for signing in', | ||
}, | ||
registerFormAccountSchoolOrganizationText: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
registerFormAccountSchoolOrganizationText: { | |
registrationFormSchoolOrOrganizationLink: { |
16605be
to
5c6c003
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the comments before merging.
src/authn-component/messages.jsx
Outdated
defaultMessage: 'By creating an account, you agree to the Terms of Service and Honor Code and you acknowledge that edX and each Member process your personal data in accordance with the Privacy Policy.', | ||
description: 'Text that appears on registration form stating honor code and privacy policy', | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra line
registrationFormHeading: { | ||
id: 'registration.form.heading', | ||
defaultMessage: 'Create account', | ||
description: 'registration form main heading', | ||
}, | ||
registrationFormOrText: { | ||
id: 'registration.form.or.message', | ||
defaultMessage: 'or', | ||
description: 'Heading that appears between social auth and basic registration form', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name similar to how we have named in Login form?
registrationFormHeading: { | |
id: 'registration.form.heading', | |
defaultMessage: 'Create account', | |
description: 'registration form main heading', | |
}, | |
registrationFormOrText: { | |
id: 'registration.form.or.message', | |
defaultMessage: 'or', | |
description: 'Heading that appears between social auth and basic registration form', | |
}, | |
registrationFormHeading1: { | |
id: 'registration.form.heading.1', | |
defaultMessage: 'Create account', | |
description: 'registration form main heading', | |
}, | |
registrationFormHeading2: { | |
id: 'registration.form.heading.2', | |
defaultMessage: 'or', | |
description: 'Heading that appears between social auth and basic registration form', | |
}, |
registrationFormCreateAccountButton: { | ||
id: 'registration.form.continue.button', | ||
defaultMessage: 'Create an account for free', | ||
description: 'Text for submit button', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: 'Text for submit button', | |
description: 'Text for submit button on registration form', |
registrationFormAlreadyHaveAccountText: { | ||
id: 'registration.form.already.have.account.text', | ||
defaultMessage: 'Already have an account?', | ||
description: 'Text for registration button', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: 'Text for registration button', | |
description: 'Login button help text', |
5c6c003
to
0f6b354
Compare
Description
In this pull request, I've implemented the registration form in alignment with the Figma style guidelines. I've included components such as the social authentication button, dividers, links, and footer text as specified in the design.
JIRA
VAN-1815
How Has This Been Tested?
Tested locally
Screenshots/sandbox (optional):
Merge Checklist