-
Notifications
You must be signed in to change notification settings - Fork 30
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
Added Button component, set the theme colors and fonts #54
base: master
Are you sure you want to change the base?
Added Button component, set the theme colors and fonts #54
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.
@blenderskool : Thank you for the quick pull request! Good job on your first component as well. I've requested a few changes, please do not get frustrated or discouraged.
I want you to think, make writing good quality code a habit, and you'll write wonderful code from the get go!
src/components/Button.js
Outdated
import { withStyles } from 'material-ui/styles'; | ||
import Button from 'material-ui/Button'; | ||
|
||
const styles = { |
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.
Please add a Button.scss
and then import './Button.css
in your file. You need to move these styles to that file.
I understand that some of you have seen a few codes where people just write CSS in JS and that works most of the time.
However, that's mostly for a very dynamic use case when you've got a lot of interactivity with your styles. There may be other schools of thought but I have always felt that separation of concerns is a good thing.
src/components/Button.js
Outdated
}; | ||
|
||
function CustomButton(props) { | ||
if (props.type === 'primary' || !props.type) { |
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.
Productivity hack with react: Use destructuring.
For example, you've got a few properties inside props
that you'd like to use.
Use them like this:
const {
type = 'primary',
onClick,
classes,
className,
children,
} = props;
// Now you don't have to do `prop.whatever` or `props.something` ever again!
src/components/Button.js
Outdated
</Button> | ||
); | ||
} | ||
else if (props.type === 'secondary') { |
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.
You don't even need these conditions 😄
You could just do something like:
<Button
color={type}
onClick={onClick}
className={`${className} ${classes[type]}`}
>
{ props.children ? props.children : 'Button' }
</Button>
if else if
conditions are not needed. Do something similar for type
=== 'flat'!
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.
Thanks for pointing this out. I completely forgot about ES6 power 😉
src/App.css
Outdated
|
||
.App-header { | ||
background-color: #222; | ||
height: 150px; | ||
padding: 20px; | ||
color: white; | ||
} | ||
color: white; } |
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.
There's a saying in the UI world.
Manners may maketh a man, but it's indentation which makes a developer!
Indent properly, move the brackets to proper places and not just your end product, but your code will look beautiful as well! 😄
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.
Sorry for this problem, but I think that was due to parsing the App.scss file. I will see if I can fix it though 👍
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.
@divyamrastogi Sorry, I cannot fix this, since this is happening with every SCSS file being converted into CSS.
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.
You're right. Everyone was facing that issue. I've added App.css to .gitignore
now. Just take a pull from master and you won't have to commit your App.css
.
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.
@divyamrastogi No, it isn't getting ignored. I think you should change it to src/App.css
@divyamrastogi I have improved the code based on your suggestions. Please let me know your thoughts 🙂 |
@divyamrastogi The code suggestions look great! Thanks for the detailed review. |
@blenderskool : I've made a few changes for you and made a PR to your branch. You can merge that and then we'll merge this. |
@divyamrastogi I see a merge conflict because of App.css file. Some help needed 😅 |
Same problem happened with me divyam helped me with this - App.css was deleted from the master. So, you don't have to push it. Just run the following in you command line: rm src/App.css && git add . && git commit |
Thanks @saini-himanshu |
Fixes #48
Checklist
master
branch.Changes proposed in this pull request:
primary | secondary | flat
defines the type of button used (default:primary
)