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 #42

Open
pradel opened this issue Apr 20, 2017 · 13 comments
Open

Redesign #42

pradel opened this issue Apr 20, 2017 · 13 comments

Comments

@pradel
Copy link
Member

pradel commented Apr 20, 2017

Opened this issue to talk about the redesign of the react package. @TimMikeladze @davidyaha

@pradel
Copy link
Member Author

pradel commented Apr 20, 2017

Right now the react and react-native diverge a lot.
The idea is to write a common logic shared between the 2 packages.
What we need to support:

  • Ui for libraries (material-ui, semantic-ui, unstyled etc..)
  • helpers components (withCurrentUser, authenticate etc...)
  • be able to extends forms ?
  • make them work with any router
  • provide an AccountsProvider which accept the accountsClient config and the theme object
  • allow internationalisation (i18n) by allowing a custom message dictionary

Idealy we should try to make the packages as light as possible

@pradel
Copy link
Member Author

pradel commented Apr 20, 2017

Here are my thought about how can work the AccountsProvider.

AccountsProvider will forward accountsClient to childs with the react context api.

When AccountsProvider is mounted set redux loading state to true. We check if there are token in the storage if no set loading to false and user to null. If yes try to see if there are valid with the server. If they are valid set loading to false and user. (this part will be handled by an AccountsClient function)

The LogInForm form component will take the config from react context and display the form with the form type (USERNAME_AND_EMAIL etc..) defined in accountsClient config.

If we let the user define the routes, the components can works with any router. The question is how to handle the redirect between the pages from 'sign-in' to 'log-in'. For example react-router use Link component or route.push('route') api.

import { AccountsClient } from 'js-accounts/client';
import { AccountsProvider } from 'js-accounts/react';
import { SignInForm, LogInForm } from 'js-accounts/react-material-ui';

const accountsClient = new AccountsClient(config);

// For react-router v3
const requireAuth = (replace, next) => {
  if (!accountsClient.isLoggedIn()) {
  // or maybe subscribe to the redux store if the app is still loading and call next when the app is ready
    replace('/login-in');
 }
 next();
}

render((
  <AccountsProvider accountsClient={accountsClient}>
    <Router history={browserHistory}>
      <Route path="login" component={LogInForm} />
      <Route path="sign-up" component={SignInForm} />
      <Route path="protected" component={MyProtectedComponent} onEnter={requireAuth} />
    </Router>
  </AccountsProvider>
), document.getElementById('root'));

// For react-router v4
const PrivateRoute = ({ component: Component, ...rest }) => (
  <Route {...rest} render={props => (
    accountsClient.isLoggedIn() ? (
      <Component {...props}/>
    ) : (
      <Redirect to={{
        pathname: '/login',
        state: { from: props.location }
      }}/>
    )
  )}/>
)

render((
  <AccountsProvider accountsClient={accountsClient}>
    <Router>
    <div>
      <ul>
        <li><Link to="/public">Public Page</Link></li>
        <li><Link to="/protected">Protected Page</Link></li>
      </ul>
      <Route path="/login" component={LogInForm}/>
      <PrivateRoute path="/protected" component={Protected}/>
    </div>
  </Router>
  </AccountsProvider>
), document.getElementById('root'));

@TimMikeladze
Copy link
Member

The internalization should be handled within the accounts core package because 1) validation messages originate there and 2) we don't want to write additional internalization code for other ui libraries (vue, angular). One thing that we do need to define is the strings for field labels and other ui pieces like "Forgot Password".

If we let the user define the routes, the components can works with any router. The question is how to handle the redirect between the pages from 'sign-in' to 'log-in'. For example react-router use Link component or route.push('route') api.

I think this can be simply solved by allowing the user to pass in a function which defines what should occur on route changes. I'd like to avoid tying this package to any specific router but we can provide recipes or even a helper function which will define the default routes if a user so chooses.

Two important pieces of functionality that I'd like to see possible

  1. Custom forms and fields
  2. Multi step forms - think of a signup form where the user is taken through multiple steps.

These can be defined by the user and then override the existing default forms when they configure the AccountsProvider.

@pradel
Copy link
Member Author

pradel commented Apr 21, 2017

I think this can be simply solved by allowing the user to pass in a function which defines what should occur on route changes. I'd like to avoid tying this package to any specific router but we can provide recipes or even a helper function which will define the default routes if a user so chooses.

This can work yes.
We can provide some examples in the docs for major routers.

Custom forms fields and multi step forms looks like they will add a lot of complexity to the forms part right now no?

@TimMikeladze
Copy link
Member

@pradel I don't think custom forms will add much complexity, after all they are written by the user and passed in as a prop. What is needed on our end is to make sure we allow the user to pass in this prop.

@davidyaha
Copy link
Member

davidyaha commented Apr 25, 2017

Hey, sorry for my late response.

I like the direction you're going in.

About custom forms, let's try to elaborate what are the reasons for writing custom/multi step forms.
From what I figured:

  • 2FA login
  • profile info on signup

For starters, these could probably be achieved by the user by just using the "AccountsProvider" and implement his own custom form.

Do you think of any other use case?

@TimMikeladze
Copy link
Member

We have a use case in our app at work where the login form has a select field with a list of different authentication servers the user can send the login request to.

@davidyaha
Copy link
Member

So in order to support that you actually need multiple AccountsClient instances or you call config multiple times right?

@TimMikeladze
Copy link
Member

TimMikeladze commented Apr 26, 2017

The way I accomplish it right now is below. Not the best way to handle it but it works for now.

export const restClient = new RestClient({
  apiHost: process.env.CONTROLLER,
  rootPath: '/accounts',
});
setAuthServer(id, url) {
    localStorage.setItem(localStorageItemId, id);
    localStorage.setItem(localStorageItemUrl, url);
    // TODO This is a hack. The client doesn't support setting the apiHost from a function
    restClient.options.apiHost = url;
  }

Ideally I'd just like to initialize a new rest client, pass it to a new accounts instance which is turn passed to the AccountsProvider

@pradel
Copy link
Member Author

pradel commented Apr 27, 2017

For the forms:

I think each ui packages should handle the update logic and call accountsClient.validate which will return an array of errors that the ui-package can show to the user.

By default it will have default className but the user will also be able to override some props:

<SignInForm layoutClassName="my-layout" errorClassName="error" ... /> 

Components needed:

  • SignInForm
  • LogInForm
  • ForgotPasswordForm
  • PaswordResetForm
  • ChangePaswordForm (not sure if we need to provide it or not)

@TimMikeladze
Copy link
Member

@pradel That sounds good. I think we should write a change password form (or maybe just the fields) since I believe we provide a changePassword mutation in the api.

@davidyaha
Copy link
Member

The list of forms looks good!
The className approach won't work react-native unfortunately..
We should maybe use style props instead? From what i've seen those work for both react-native and react.
Are there any disadvantages to style that you know of?

@pradel
Copy link
Member Author

pradel commented May 14, 2017

In react we can't do media-queries with style approach but we can do style + classname for react and style only for react-native since it will be handled by the differents ui packages

@pradel pradel mentioned this issue May 18, 2017
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

No branches or pull requests

3 participants