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

[WIP] Feature - Welcome Screen #138

Merged
merged 28 commits into from Jan 25, 2018

Conversation

arthurdenner
Copy link
Contributor

@arthurdenner arthurdenner commented Jan 13, 2018

Hello, everybody.

This PR is not finished, as the title says, but I wanted to share what I've built so far to get feedback and to ask a question regarding #22.

So, I started working on this feature with the components that already existed, but after a while, I started to not like the result, so I decided to start almost from the scratch.

So far, I've added a simple page with some information about the project (pure copy & paste from GitHub and the website) and the forms for sign in and sign up. The following things are missing:

  • language options: I still need to see how to get the languages available. It shouldn't be hard, I just haven't done it yet;
  • passwordConfirm validation: There is a problem in Yup to make this validation, but they're working on it;
  • integration with the API: I'm waiting on the API to be live so I can finish this.

My question is:

  • Are we going to support translation for the welcome screen? I feel like we should, but since we don't have all the symbols proofread, maybe we shouldn't rely in automatic translation for this, I don't know.

That's it for now. Please feel free to comment and leave your opinion about the UI or anything else.

@shayc
Copy link
Collaborator

shayc commented Jan 15, 2018

  • Language options: you can check Redux state language.langs
  • Ok
  • @martinbedouret can update estimations on API

The WelcomeScreen should support translations, we will proofread the entire app when time comes.

Copy link
Collaborator

@shayc shayc left a comment

Choose a reason for hiding this comment

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

Good PR! regarding translation, if you want I can do it.. let me know 😃

package.json Outdated
@@ -18,6 +18,7 @@
"private": false,
"dependencies": {
"browser-image-resizer": "^1.1.3",
"formik": "^0.10.5",
Copy link
Collaborator

Choose a reason for hiding this comment

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

formik is cool!

@@ -1,3 +1,7 @@
// action types constants
export const FINISH_FIRST_VISIT = 'cboard/Speech/FINISH_FIRST_VISIT';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be'cboard/App/FINISH_FIRST_VISIT'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, copy and paste 😅

import { FormControl, FormHelperText } from 'material-ui/Form';

const TextField = ({ error, ...props }) => (
<FormControl className="SignUp__field" error={!!error}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

classname SignUp__field suggests a SignUp component, yet the filename/path suggest generic TextField. If you intend to keep the FormItems generic, you should change the classname and have the CSS files inside each FormItems component.

import Typography from 'material-ui/Typography';

const Information = () => (
<Fragment>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Way to go for using Fragment!

@@ -0,0 +1,20 @@
.Login__field:not(:first-child) {
margin-top: 0.75em !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid !important, they're contagious, if this is because you need to run over MUI styles, I'd rather you'd override at the JS level, inside the component (only for MUI related)

}

.Login__field_gender {
margin-top: 1em !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here for !important

@@ -1,8 +1,20 @@
.SignUp {
.SignUp__field:not(:first-child) {
margin-top: 0.75em !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

!important

}

.SignUp__heading {
.SignUp__field_gender {
margin-top: 1em !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

!important

/>
</div>
<div className="WelcomeScreen__footer">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use semantic <footer> element,

@arthurdenner
Copy link
Contributor Author

arthurdenner commented Jan 15, 2018

@shayc, I've updated the branch with what you reviewed.
If you can do the translation part, I'd appreciate. 😃

}

.WelcomeScreen__heading {
color: #eceff1 !important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To use this color, we can't avoid this !important. 😕

A constants file was created to determine the API URL in development and production
axios was installed to handle the API calls nicely
It was created a LoadingIcon component based on the MUI CircularProgress
Login and Signup were moved to .component files
Some fields in the Signup form were removed because they're not in the user model at the moment
Were created containers and actions for the Login and Signup
The App reducer was modified to contain loading status for Login and Signup
@shayc shayc merged commit 1dd115c into cboard-org:master Jan 25, 2018
@shayc
Copy link
Collaborator

shayc commented Jan 25, 2018

looking good!

@arthurdenner arthurdenner deleted the feature/welcome-screen-v2 branch March 23, 2018 16:13
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.

None yet

2 participants