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

Create Account and Log In tabs (GH-1329, GH-1330) #180

Merged
merged 26 commits into from Sep 11, 2018
Merged

Conversation

@zarembsky
Copy link
Contributor

@zarembsky zarembsky commented Aug 31, 2018

  • [x ] Have you followed the guidelines in CONTRIBUTING.md?
  • [x ] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • [x ] Have you added an explanation of what your changes do?
  • Does your submission pass tests?
  • [ x] Did you lint your code prior to submission?
@zarembsky zarembsky requested review from IAmThePan, jsignanini, trickpattyFH20 and ghostery/ghostery as code owners Aug 31, 2018
@zarembsky zarembsky changed the base branch from master to develop Aug 31, 2018
zarembsky and others added 4 commits Aug 31, 2018
@IAmThePan
Copy link
Contributor

@IAmThePan IAmThePan commented Sep 4, 2018

@zarembsky I added a commit with:

  • Updated comments across files,
  • A new ExitButton in shared-components and I updated CreateAccountView and LogInView to use that instead of SetupNavigation__exitButton classes
  • I renamed CreateAccountView.jsx -> CreateAccountViewContainer.jsx
  • I renamed LogInView.jsx -> LogInViewContainer.jsx

Please update:

  1. On the Home page, update Create Account link with the user's email address if the user is signed in.
  2. In the Side Navigation, update the Create Account and Sign In links with the email address and Sign Out when the user is signed in.
    2a. Sign Out should work.
  3. Upon successful login or registration, navigate the user to the Home page.
    3a. For Log In only (not create account) if the user has the justInstalled query parameter, remove it.
  4. Move the complicated rendering out of CreateAccountViewContainer and LogInViewContainer and into new files: CreateAccountview and LogInView

Thanks

Copy link
Contributor

@IAmThePan IAmThePan left a comment

Approved.

zarembsky added 2 commits Sep 10, 2018
@@ -34,6 +34,7 @@ class HomeViewContainer extends React.Component {
window.document.title = title;

this.props.actions.getHomeProps();
props.actions.getUser();

This comment has been minimized.

@jsignanini

jsignanini Sep 11, 2018
Member

let's use this.props here for consistency.

setToast,
register: AccountActions.register,
getUser: AccountActions.getUser,
updateAccountPromotions: AccountActions.updateAccountPromotions,

This comment has been minimized.

@jsignanini

jsignanini Sep 11, 2018
Member

let's write this as Object.assign({}, AccountActions, { setToast }), if we want all AccountActions functions. Otherwise, let's only import the required actions instead of all the AccountActions.

@@ -33,7 +34,9 @@ const mapStateToProps = state => Object.assign({}, state.home);
* @memberof SetupContainers
*/
const mapDispatchToProps = dispatch => ({
actions: bindActionCreators(Object.assign(HomeViewActions), dispatch),
actions: bindActionCreators(Object.assign({}, HomeViewActions, {
getUser: AccountActions.getUser,

This comment has been minimized.

@jsignanini

jsignanini Sep 11, 2018
Member

let's only import the function we need here, and not all the AccountActions.

login: AccountActions.login,
getUser: AccountActions.getUser,
getUserSettings: AccountActions.getUserSettings,
}), dispatch),

This comment has been minimized.

@jsignanini

jsignanini Sep 11, 2018
Member

same as above here

actions: bindActionCreators(Object.assign({}, {
setToast,
getUser: AccountActions.getUser,
logout: AccountActions.logout,

This comment has been minimized.

@jsignanini

jsignanini Sep 11, 2018
Member

same here, let's only import the functions we are using.

@@ -32,7 +32,9 @@ const mapStateToProps = state => Object.assign({}, state.account);
* @memberof TutorialContainers
*/
const mapDispatchToProps = dispatch => ({
actions: bindActionCreators(Object.assign({}, AccountActions), dispatch),
actions: bindActionCreators(Object.assign({}, {
getUser: AccountActions.getUser,

This comment has been minimized.

@jsignanini

jsignanini Sep 11, 2018
Member

same here, we can just import getUser

IAmThePan added 2 commits Sep 11, 2018
@jsignanini jsignanini merged commit 821a281 into develop Sep 11, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jsignanini jsignanini deleted the feature/account branch Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants