From bbe0bf970fee1995ecce20bf788da3818c70064d Mon Sep 17 00:00:00 2001 From: Vaidehi Joshi Date: Wed, 15 Apr 2020 15:16:53 -0700 Subject: [PATCH] Split up content of EmailListTermsConditionsForm within onboarding (#7294) [deploy] * Move Terms & Conditions, CoC into Intro slide of onboarding Closes https://github.com/thepracticaldev/dev.to/issues/6545. * Rename ClosingSlide in Onboarding tests * Rename EmailListTermsConditionsForm to EmailPreferencesForm * Proper lint and prepend `v2:` to some onboarding components * Move email preferences to last slide Closes https://github.com/thepracticaldev/dev.to/issues/6550. * Use crayons variables for padding * Remove backToSlide function, rename buttonIsDisabled function --- .../stylesheets/preact/onboarding-modal.scss | 22 +- app/javascript/onboarding/Onboarding.jsx | 4 +- .../onboarding/__tests__/Onboarding.test.jsx | 114 ++++----- .../__snapshots__/Onboarding.test.jsx.snap | 126 +++++----- .../EmailListTermsConditionsForm.jsx | 223 ------------------ .../components/EmailPreferencesForm.jsx | 111 +++++++++ .../onboarding/components/FollowTags.jsx | 2 +- .../onboarding/components/FollowUsers.jsx | 2 +- .../onboarding/components/IntroSlide.jsx | 135 ++++++++++- .../onboarding/components/Navigation.jsx | 32 ++- .../onboarding/components/ProfileForm.jsx | 11 +- 11 files changed, 414 insertions(+), 368 deletions(-) delete mode 100644 app/javascript/onboarding/components/EmailListTermsConditionsForm.jsx create mode 100644 app/javascript/onboarding/components/EmailPreferencesForm.jsx diff --git a/app/assets/stylesheets/preact/onboarding-modal.scss b/app/assets/stylesheets/preact/onboarding-modal.scss index 6a47969bf..eadc4c57b 100644 --- a/app/assets/stylesheets/preact/onboarding-modal.scss +++ b/app/assets/stylesheets/preact/onboarding-modal.scss @@ -109,11 +109,19 @@ width: 100%; border-bottom: 2px solid var(--box-darker); + &.intro-slide { + border-bottom: none; + } + .navigation-content { display: flex; justify-content: space-between; padding: $su-4; + &.intro-slide { + padding: 0; + } + button { @extend .crayons-btn; } @@ -337,6 +345,10 @@ padding: $su-7; justify-content: center; + &.intro-slide { + padding: 0; + } + button { @extend .crayons-btn; } @@ -586,6 +598,14 @@ $onboarding-user-selected-hover: rgba(71, 85, 235, 0.2); } } + .checkbox-form-wrapper { + padding: $su-7; + + .checkbox-form { + padding-bottom: $su-6; + } + } + // Checkboxes .checkbox-item { label { @@ -625,5 +645,5 @@ $onboarding-user-selected-hover: rgba(71, 85, 235, 0.2); .onboarding-form-input--last { margin-bottom: 25px; } - } + } } diff --git a/app/javascript/onboarding/Onboarding.jsx b/app/javascript/onboarding/Onboarding.jsx index b0fbff0e3..0cb3f3665 100644 --- a/app/javascript/onboarding/Onboarding.jsx +++ b/app/javascript/onboarding/Onboarding.jsx @@ -2,7 +2,7 @@ import 'preact/devtools'; import { h, Component } from 'preact'; import IntroSlide from './components/IntroSlide'; -import EmailListTermsConditionsForm from './components/EmailListTermsConditionsForm'; +import EmailPreferencesForm from './components/EmailPreferencesForm'; import ClosingSlide from './components/ClosingSlide'; import FollowTags from './components/FollowTags'; import FollowUsers from './components/FollowUsers'; @@ -20,10 +20,10 @@ export default class Onboarding extends Component { const slides = [ IntroSlide, - EmailListTermsConditionsForm, ProfileForm, FollowTags, FollowUsers, + EmailPreferencesForm, ClosingSlide, ]; diff --git a/app/javascript/onboarding/__tests__/Onboarding.test.jsx b/app/javascript/onboarding/__tests__/Onboarding.test.jsx index 83e243493..2c05b3218 100644 --- a/app/javascript/onboarding/__tests__/Onboarding.test.jsx +++ b/app/javascript/onboarding/__tests__/Onboarding.test.jsx @@ -5,7 +5,6 @@ import { axe, toHaveNoViolations } from 'jest-axe'; import Onboarding from '../Onboarding'; import ProfileForm from '../components/ProfileForm'; -import EmailTermsConditionsForm from '../components/EmailListTermsConditionsForm'; import FollowTags from '../components/FollowTags'; import FollowUsers from '../components/FollowUsers'; @@ -82,29 +81,6 @@ describe('', () => { describe('IntroSlide', () => { let onboardingSlides; - - beforeEach(() => { - onboardingSlides = initializeSlides(0, getUserData()); - }); - - test('renders properly', () => { - expect(onboardingSlides).toMatchSnapshot(); - }); - - test('should not have basic a11y violations', async () => { - const results = await axe(onboardingSlides.toString()); - - expect(results).toHaveNoViolations(); - }); - - test('should advance', () => { - onboardingSlides.find('.next-button').simulate('click'); - expect(onboardingSlides.state().currentSlide).toBe(1); - }); - }); - - describe('EmailTermsConditionsForm', () => { - let onboardingSlides; const codeOfConductCheckEvent = { target: { value: 'checked_code_of_conduct', @@ -117,6 +93,7 @@ describe('', () => { name: 'checked_terms_and_conditions', }, }; + const updateCodeOfConduct = () => { onboardingSlides .find('#checked_code_of_conduct') @@ -129,59 +106,29 @@ describe('', () => { }; beforeEach(() => { - onboardingSlides = initializeSlides(1, getUserData()); + onboardingSlides = initializeSlides(0, getUserData()); }); test('renders properly', () => { expect(onboardingSlides).toMatchSnapshot(); }); - // Arguably this test is actually just testing the Preact framework - // but for the sake of detecting a regression I am refactoring it instead - // of removing it (@jacobherrington) - test('should track state changes', () => { - const emailTerms = onboardingSlides.find(); - - expect(emailTerms.state('checked_code_of_conduct')).toBe(false); - expect(emailTerms.state('checked_terms_and_conditions')).toBe(false); - - updateCodeOfConduct(); - updateTermsAndConditions(); - - expect(emailTerms.state('checked_code_of_conduct')).toBe(true); - expect(emailTerms.state('checked_terms_and_conditions')).toBe(true); - }); - - test('should not advance if required boxes are not checked', () => { - // When none of the boxes are checked - onboardingSlides.find('.next-button').simulate('click'); - expect(onboardingSlides.state().currentSlide).toBe(1); - - // When only the code of conduct is checked - updateCodeOfConduct(); - expect(onboardingSlides.state().currentSlide).toBe(1); - - // When only the terms and conditions are checked - updateCodeOfConduct(); - updateTermsAndConditions(); - onboardingSlides.find('.next-button').simulate('click'); - expect(onboardingSlides.state().currentSlide).toBe(1); - }); - test('should advance if required boxes are checked', async () => { fetch.once({}); + expect(onboardingSlides.state().currentSlide).toBe(0); updateCodeOfConduct(); updateTermsAndConditions(); onboardingSlides.find('.next-button').simulate('click'); await flushPromises(); - expect(onboardingSlides.state().currentSlide).toBe(2); + expect(onboardingSlides.state().currentSlide).toBe(1); }); - it('should step backward', () => { - onboardingSlides.find('.back-button').simulate('click'); - expect(onboardingSlides.state().currentSlide).toBe(0); + test('should not have basic a11y violations', async () => { + const results = await axe(onboardingSlides.toString()); + + expect(results).toHaveNoViolations(); }); }); @@ -193,7 +140,7 @@ describe('', () => { document.body.appendChild(meta); beforeEach(() => { - onboardingSlides = initializeSlides(2, getUserData()); + onboardingSlides = initializeSlides(1, getUserData()); }); test('renders properly', () => { @@ -231,12 +178,12 @@ describe('', () => { profileForm.find('.next-button').simulate('click'); fetch.once(fakeTagsResponse); await flushPromises(); - expect(onboardingSlides.state().currentSlide).toBe(3); + expect(onboardingSlides.state().currentSlide).toBe(2); }); it('should step backward', () => { onboardingSlides.find('.back-button').simulate('click'); - expect(onboardingSlides.state().currentSlide).toBe(1); + expect(onboardingSlides.state().currentSlide).toBe(0); }); }); @@ -244,7 +191,7 @@ describe('', () => { let onboardingSlides; beforeEach(async () => { - onboardingSlides = initializeSlides(3, getUserData(), fakeTagsResponse); + onboardingSlides = initializeSlides(2, getUserData(), fakeTagsResponse); await flushPromises(); }); @@ -256,7 +203,7 @@ describe('', () => { expect(onboardingSlides.find('.onboarding-tags__item').length).toBe(3); }); - test('should allow a user to add a tag', async () => { + test('should allow a user to add a tag and advance', async () => { fetch.once({}); const followTags = onboardingSlides.find(); const firstButton = onboardingSlides @@ -269,12 +216,12 @@ describe('', () => { onboardingSlides.find('.next-button').simulate('click'); fetch.once(fakeUsersResponse); await flushPromises(); - expect(onboardingSlides.state().currentSlide).toBe(4); + expect(onboardingSlides.state().currentSlide).toBe(3); }); it('should step backward', () => { onboardingSlides.find('.back-button').simulate('click'); - expect(onboardingSlides.state().currentSlide).toBe(2); + expect(onboardingSlides.state().currentSlide).toBe(1); }); }); @@ -282,7 +229,7 @@ describe('', () => { let onboardingSlides; beforeEach(async () => { - onboardingSlides = initializeSlides(4, getUserData(), fakeUsersResponse); + onboardingSlides = initializeSlides(3, getUserData(), fakeUsersResponse); await flushPromises(); }); @@ -309,7 +256,7 @@ describe('', () => { expect(followUsers.state('selectedUsers').length).toBe(2); onboardingSlides.find('.next-button').simulate('click'); await flushPromises(); - expect(onboardingSlides.state().currentSlide).toBe(5); + expect(onboardingSlides.state().currentSlide).toBe(4); }); test('should have a functioning select-all toggle', async () => { @@ -330,11 +277,36 @@ describe('', () => { fetch.once(fakeTagsResponse); onboardingSlides.find('.back-button').simulate('click'); await flushPromises(); + expect(onboardingSlides.state().currentSlide).toBe(2); + }); + }); + + describe('EmailPreferencesForm', () => { + let onboardingSlides; + + beforeEach(() => { + onboardingSlides = initializeSlides(4, getUserData()); + }); + + test('renders properly', () => { + expect(onboardingSlides).toMatchSnapshot(); + }); + + test('should allow user to advance', async () => { + fetch.once({}); + + onboardingSlides.find('.next-button').simulate('click'); + await flushPromises(); + expect(onboardingSlides.state().currentSlide).toBe(5); + }); + + it('should step backward', () => { + onboardingSlides.find('.back-button').simulate('click'); expect(onboardingSlides.state().currentSlide).toBe(3); }); }); - describe('CloseSlide', () => { + describe('ClosingSlide', () => { let onboardingSlides; beforeEach(() => { diff --git a/app/javascript/onboarding/__tests__/__snapshots__/Onboarding.test.jsx.snap b/app/javascript/onboarding/__tests__/__snapshots__/Onboarding.test.jsx.snap index d9dd01c80..ef85e93d5 100644 --- a/app/javascript/onboarding/__tests__/__snapshots__/Onboarding.test.jsx.snap +++ b/app/javascript/onboarding/__tests__/__snapshots__/Onboarding.test.jsx.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[` CloseSlide renders properly 1`] = ` +exports[` ClosingSlide renders properly 1`] = ` preact-render-spy (1 nodes) -------
@@ -77,7 +77,7 @@ preact-render-spy (1 nodes) `; -exports[` EmailTermsConditionsForm renders properly 1`] = ` +exports[` EmailPreferencesForm renders properly 1`] = ` preact-render-spy (1 nodes) -------
@@ -102,53 +102,12 @@ preact-render-spy (1 nodes)
-

Getting started

-

Let's review a few things first

+

Almost there!

+

+ Review your email preferences before we continue. +

-
- Some things to check off - -
Email preferences
    @@ -173,7 +132,7 @@ preact-render-spy (1 nodes) checked={true} onChange={[Function bound handleChange]} /> - I want to receive a periodic digest with some of the top posts from your tags. + I want to receive a periodic digest of top posts from my tags.
@@ -413,17 +372,66 @@ preact-render-spy (1 nodes) DEV is where programmers share ideas and help each other grow.
- +
+ +
+ +
+ + +
diff --git a/app/javascript/onboarding/components/EmailListTermsConditionsForm.jsx b/app/javascript/onboarding/components/EmailListTermsConditionsForm.jsx deleted file mode 100644 index 8ec704b7f..000000000 --- a/app/javascript/onboarding/components/EmailListTermsConditionsForm.jsx +++ /dev/null @@ -1,223 +0,0 @@ -/* eslint camelcase: "off" */ - -import { h, Component } from 'preact'; -import PropTypes from 'prop-types'; - -import Navigation from './Navigation'; -import { getContentOfToken, updateOnboarding } from '../utilities'; - -/* eslint-disable camelcase */ - -class EmailTermsConditionsForm extends Component { - constructor(props) { - super(props); - - this.handleChange = this.handleChange.bind(this); - this.onSubmit = this.onSubmit.bind(this); - this.checkRequirements = this.checkRequirements.bind(this); - - this.state = { - checked_code_of_conduct: false, - checked_terms_and_conditions: false, - email_newsletter: true, - email_digest_periodic: true, - message: '', - textShowing: null, - }; - } - - componentDidMount() { - updateOnboarding('emails, COC and T&C form'); - } - - onSubmit() { - if (!this.checkRequirements()) return; - const csrfToken = getContentOfToken('csrf-token'); - - fetch('/onboarding_checkbox_update', { - method: 'PATCH', - headers: { - 'X-CSRF-Token': csrfToken, - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ user: this.state }), - credentials: 'same-origin', - }).then((response) => { - if (response.ok) { - localStorage.setItem('shouldRedirectToOnboarding', false); - const { next } = this.props; - next(); - } - }); - } - - checkRequirements() { - const { - checked_code_of_conduct, - checked_terms_and_conditions, - } = this.state; - if (!checked_code_of_conduct) { - this.setState({ - message: 'You must agree to our Code of Conduct before continuing.', - }); - return false; - } - if (!checked_terms_and_conditions) { - this.setState({ - message: - 'You must agree to our Terms and Conditions before continuing.', - }); - return false; - } - return true; - } - - handleChange(event) { - const { name } = event.target; - this.setState((currentState) => ({ - [name]: !currentState[name], - })); - } - - handleShowText(event, id) { - event.preventDefault(); - this.setState({ textShowing: document.getElementById(id).innerHTML }); - } - - backToSlide() { - this.setState({ textShowing: null }); - } - - render() { - const { - message, - checked_code_of_conduct, - checked_terms_and_conditions, - email_newsletter, - email_digest_periodic, - textShowing, - } = this.state; - const { prev } = this.props; - if (textShowing) { - return ( -
-
- -
-
-
- ); - } - return ( -
- -
-
-

Getting started

-

Let's review a few things first

-
- - {message && ( - - {message} - - )} - -
-
- Some things to check off - -
- -
- Email preferences -
    -
  • - -
    • -
  • - -
    • -
-
-
-
-
- ); - } -} - -EmailTermsConditionsForm.propTypes = { - prev: PropTypes.func.isRequired, - next: PropTypes.string.isRequired, -}; - -export default EmailTermsConditionsForm; diff --git a/app/javascript/onboarding/components/EmailPreferencesForm.jsx b/app/javascript/onboarding/components/EmailPreferencesForm.jsx new file mode 100644 index 000000000..17ca42552 --- /dev/null +++ b/app/javascript/onboarding/components/EmailPreferencesForm.jsx @@ -0,0 +1,111 @@ +import { h, Component } from 'preact'; +import PropTypes from 'prop-types'; + +import Navigation from './Navigation'; +import { getContentOfToken, updateOnboarding } from '../utilities'; + +/* eslint-disable camelcase */ +class EmailPreferencesForm extends Component { + constructor(props) { + super(props); + + this.handleChange = this.handleChange.bind(this); + this.onSubmit = this.onSubmit.bind(this); + + this.state = { + email_newsletter: true, + email_digest_periodic: true, + }; + } + + componentDidMount() { + updateOnboarding('v2: email preferences form'); + } + + onSubmit() { + const csrfToken = getContentOfToken('csrf-token'); + + fetch('/onboarding_checkbox_update', { + method: 'PATCH', + headers: { + 'X-CSRF-Token': csrfToken, + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ user: this.state }), + credentials: 'same-origin', + }).then((response) => { + if (response.ok) { + localStorage.setItem('shouldRedirectToOnboarding', false); + const { next } = this.props; + next(); + } + }); + } + + handleChange(event) { + const { name } = event.target; + this.setState((currentState) => ({ + [name]: !currentState[name], + })); + } + + render() { + const { email_newsletter, email_digest_periodic } = this.state; + const { prev } = this.props; + return ( +
+ +
+
+

Almost there!

+

+ Review your email preferences before we continue. +

+
+ +
+
+ Email preferences +
    +
  • + +
    • +
  • + +
    • +
+
+
+
+
+ ); + } +} + +EmailPreferencesForm.propTypes = { + prev: PropTypes.func.isRequired, + next: PropTypes.string.isRequired, +}; + +export default EmailPreferencesForm; + +/* eslint-enable camelcase */ diff --git a/app/javascript/onboarding/components/FollowTags.jsx b/app/javascript/onboarding/components/FollowTags.jsx index 5eeb6028a..0ff9a91aa 100644 --- a/app/javascript/onboarding/components/FollowTags.jsx +++ b/app/javascript/onboarding/components/FollowTags.jsx @@ -32,7 +32,7 @@ class FollowTags extends Component { 'Content-Type': 'application/json', }, body: JSON.stringify({ - user: { last_onboarding_page: 'follow tags page' }, + user: { last_onboarding_page: 'v2: follow tags page' }, }), credentials: 'same-origin', }); diff --git a/app/javascript/onboarding/components/FollowUsers.jsx b/app/javascript/onboarding/components/FollowUsers.jsx index e2e91840f..fadae911d 100644 --- a/app/javascript/onboarding/components/FollowUsers.jsx +++ b/app/javascript/onboarding/components/FollowUsers.jsx @@ -38,7 +38,7 @@ class FollowUsers extends Component { 'Content-Type': 'application/json', }, body: JSON.stringify({ - user: { last_onboarding_page: 'follow users page' }, + user: { last_onboarding_page: 'v2: follow users page' }, }), credentials: 'same-origin', }); diff --git a/app/javascript/onboarding/components/IntroSlide.jsx b/app/javascript/onboarding/components/IntroSlide.jsx index b1b3f1418..44292fc61 100644 --- a/app/javascript/onboarding/components/IntroSlide.jsx +++ b/app/javascript/onboarding/components/IntroSlide.jsx @@ -2,27 +2,94 @@ import { h, Component } from 'preact'; import PropTypes from 'prop-types'; import Navigation from './Navigation'; -import { userData, updateOnboarding } from '../utilities'; +import { getContentOfToken, userData, updateOnboarding } from '../utilities'; +/* eslint-disable camelcase */ class IntroSlide extends Component { constructor(props) { super(props); + this.handleChange = this.handleChange.bind(this); this.onSubmit = this.onSubmit.bind(this); this.user = userData(); + + this.state = { + checked_code_of_conduct: false, + checked_terms_and_conditions: false, + text: null, + }; } componentDidMount() { - updateOnboarding('intro slide'); + updateOnboarding('v2: intro, code of conduct, terms & conditions'); } onSubmit() { const { next } = this.props; - next(); + const csrfToken = getContentOfToken('csrf-token'); + + fetch('/onboarding_checkbox_update', { + method: 'PATCH', + headers: { + 'X-CSRF-Token': csrfToken, + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ user: this.state }), + credentials: 'same-origin', + }).then((response) => { + if (response.ok) { + localStorage.setItem('shouldRedirectToOnboarding', false); + next(); + } + }); + } + + handleChange(event) { + const { name } = event.target; + this.setState((currentState) => ({ + [name]: !currentState[name], + })); + } + + handleShowText(event, id) { + event.preventDefault(); + this.setState({ text: document.getElementById(id).innerHTML }); + } + + isButtonDisabled() { + const { + checked_code_of_conduct, + checked_terms_and_conditions, + } = this.state; + + return !checked_code_of_conduct || !checked_terms_and_conditions; } render() { const { prev } = this.props; + const { + checked_code_of_conduct, + checked_terms_and_conditions, + text, + } = this.state; + + if (text) { + return ( +
+
+ +
+
+
+ ); + } return (
@@ -43,7 +110,65 @@ class IntroSlide extends Component { DEV is where programmers share ideas and help each other grow.
- + +
+
+
+ +
+
+ +
); } @@ -55,3 +180,5 @@ IntroSlide.propTypes = { }; export default IntroSlide; + +/* eslint-enable camelcase */ diff --git a/app/javascript/onboarding/components/Navigation.jsx b/app/javascript/onboarding/components/Navigation.jsx index 92b11e326..0280e9136 100644 --- a/app/javascript/onboarding/components/Navigation.jsx +++ b/app/javascript/onboarding/components/Navigation.jsx @@ -1,16 +1,36 @@ import { h } from 'preact'; import PropTypes from 'prop-types'; -const Navigation = ({ next, prev, hideNext, hidePrev }) => ( -