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

redesign #44

Closed
wants to merge 1 commit into from
Closed

redesign #44

wants to merge 1 commit into from

Conversation

pradel
Copy link
Member

@pradel pradel commented May 18, 2017

Here is a first try as discussed in #42.

  • AccountsProvider
  • withCurrentUser
  • login + signup form for material-ui

Related pr for client accounts-js/accounts#104
Related pr for example accounts-js/examples#7

Right now the example is working well, the signup + login form is working and the tokens are stored and consumed when the app start.

I limited the dependencies for the packages to try to be lighter as possible.

withCurrentUser:
Pass down a currentUser + loading prop (when the app is starting)

const Home = withCurrentUser(({ currentUser, loading }) => (
  <div style={{ textAlign: 'center' }}>
    {loading && <CircularProgress />}
    {!loading && <AppBar
      title="js-accounts rest example"
      showMenuIconButton={false}
      iconElementRight={
        !currentUser
          ? <FlatButton label="Login" onTouchTap={() => history.push('/login')} />
          : <FlatButton label="Logout" onTouchTap={() => accountsClient.logout()} />
      }
    />}
    {!loading &&
      !currentUser &&
      <div style={{ marginTop: 40 }}>
        <p>Hey you are not logged in</p>
      </div>}
    {!loading &&
      currentUser &&
      <div style={{ marginTop: 40 }}>
        <p>Signed in user info:</p>
        <p>id : {currentUser.id}</p>
        <p>email : {currentUser.emails[0].address}</p>
      </div>}
  </div>
));

@pradel pradel requested review from TimMikeladze and davidyaha May 18, 2017 09:16
Copy link
Member

@davidyaha davidyaha left a comment

Choose a reason for hiding this comment

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

So I really like the fact that this new implementation is much much more compact :)
One issue I have in general is that we've tried to push the state back to the accounts package but instead we moved it to the components.. It seems like we should figure out a way to get rid of the stateful components altogether... not sure if there is a good way but maybe if we expose the store from the accounts client, and just use the react-redux Provider as the returned value from AccountsProvider we can use connect for the signUp and logIn forms.
Also we are missing the reset password still right?

import Paper from 'material-ui/Paper';

const Layout = ({ children }) => (
<Paper style={{ margin: 'auto', maxWidth: 400, marginTop: 50, padding: 20 }}>
Copy link
Member

Choose a reason for hiding this comment

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

So this is the default layout right? but then, I don't see any way of overriding it in a comfortable way

Copy link
Member Author

Choose a reason for hiding this comment

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

  • We can add a LayoutStyle prop
  • We can add a Layout prop that can overwrite the package Layout with a user Component

const { accountsClient } = this.context;
const { form } = this.state;
this.setState({ formError: null, errors: {} });
const errors = validate.logIn(accountsClient, form);
Copy link
Member

Choose a reason for hiding this comment

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

I think that loginWithPassword already handles validation isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes it should handle validation. All validation should be handled by @accounts/client

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay will take a look

}
try {
await accountsClient.loginWithPassword(form.user, form.password);
accountsClient.changeRoute(accountsClient.options.homePath);
Copy link
Member

Choose a reason for hiding this comment

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

The route thing should really be part of the base react package I think..


componentDidMount() {
const { accountsClient } = this.context;
this.unsubscribe = this.context.accountsClient.store.subscribe(() => {
Copy link
Member

Choose a reason for hiding this comment

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

There is no way to make the subscribe callback more specific? like only try to set state if an action with user or loading was dispatched?
I think that this way we will cause a re-render for each component that uses that for every action..

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right I really don't know I will take a look at that

Copy link
Member Author

Choose a reason for hiding this comment

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

Or if we use react-redux this can be done easily

const { accountsClient } = this.context;
const { form } = this.state;
this.setState({ formError: null, errors: {} });
const errors = validate.logIn(accountsClient, form);
Copy link
Member

Choose a reason for hiding this comment

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

Yes it should handle validation. All validation should be handled by @accounts/client

const { form, formError } = this.state;
const { passwordSignupFields } = this.context.accountsClient.options;
return (
<Layout>
Copy link
Member

Choose a reason for hiding this comment

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

This logic can be moved to the core react package.

Copy link
Member Author

Choose a reason for hiding this comment

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

?

@TimMikeladze
Copy link
Member

Thanks for the PR @pradel

I've been thinking about the redesign of both this package and the core accounts package. Before beginning any redesigns I think we should get our typing and building straightened out. See accounts-js/accounts#44 for discussion about moving to TS.

Also, I think the react packages can be written in such a way that they never depend on @accounts/client but instead are passed in functions as props (which probably come from @accounts/client).

@davidyaha
Copy link
Member

Also, I think the react packages can be written in such a way that they never depend on @accounts/client but instead are passed in functions as props (which probably come from @accounts/client).

It is great for modularity but It will (probably, or at least the way I think of possible implementation) also complicate the usage by a lot.
One possible way to approach it will be with using object spreading

<LogInForm {...accountClient} />

This will probably render the provider useless..

@pradel pradel closed this Mar 28, 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
Development

Successfully merging this pull request may close these issues.

3 participants