-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/become trainer #30
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V. small changes, this PR is fantastic! 🎖
}); | ||
}); | ||
|
||
describe('when ApiService return confirmation', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'when ApiService returns confirmation'
export const ActionButton = styled.button` | ||
width: 201px; | ||
height: 40px; | ||
background-color: #e6cf42; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable for this color?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason to do so. Action button is meant to be a "Primary Action Button" (CTA) and it has one, special color as on designs. Primary / secondary colors should rather be a part of theme which styled components support
src/lib/PrimaryActionButton.js
Outdated
const PrimaryButton = props => ( | ||
<ActionButton {...props}> | ||
{' '} | ||
<ActionButtonSpanText>{props.children}</ActionButtonSpanText>{' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is somehow ugly, maybe new line can help for the space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a leftover, will remove!
.then(user => { | ||
if (user) { | ||
this.props.loginSuccess(user); | ||
// TODO how to? this.history.push('/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know! Don't ask me 😄
return ( | ||
<div> | ||
{alreadyAttending && ( | ||
<PrimaryButton onClick={this.leave}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make a component from it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its not reusable anywhere and most likely will not be. We already have a nice reusable component of PrimaryButton
. Could you please point out what income would be from making it a component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
/callback?token=...
stuff implied login error as token was already used onceopened task for future: