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

Add menu bar buttons #4

Closed
wants to merge 2 commits into from
Closed

Add menu bar buttons #4

wants to merge 2 commits into from

Conversation

GarrySan
Copy link

No description provided.

@GarrySan
Copy link
Author

#3

Copy link
Collaborator

@aloksingh3112 aloksingh3112 left a comment

Choose a reason for hiding this comment

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

Hey Thanks For raising the pr i have added some comments please fix them.

import React from 'react'

class Taskbar extends React.Component {
clearMarkers() {
Copy link
Collaborator

@aloksingh3112 aloksingh3112 Mar 26, 2020

Choose a reason for hiding this comment

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

make this an arrow function as with onClick the "this" property changes this code doesn't throws any error for now but we will definitely be using the this property in these functions.

clearMarkers() {
console.log('clearing markers')
}
addRaw() {
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 make it a arrow function.

addRaw() {
console.log('Adding raw observations')
}
pull() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

make it a arrow function.

pull() {
console.log('Pulling from google sheets')
}
refresh() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

make it a arrow function.

refresh() {
console.log('Refreshing markers')
}
openSheet() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

make it a arrow function.

@aloksingh3112
Copy link
Collaborator

Also @ivanoats as we are on the recent version of react we should be using the best strategies that current version of react has, like we can use react-hooks to decouple our logic from our component we can use the power of custom hooks for that.
Also with react hooks code looks more cleaner and all the trouble of handling state in most of the component life cycle hooks is removed.

Also about the css if we are planning to use react-material-ui in project we wont be requiring any external css files that fixes alignment and all in hacky ways.
Also react-material-ui is very much customizable so its a win win situation for us if we use that.

@ivanoats
Copy link
Member

I agree - function components and hooks if possible.

@ivanoats
Copy link
Member

This PR needs to be closed, or refactored from a class based component to a functional / hooks style.

Base automatically changed from master to main March 6, 2021 16:57
@sahil-shubham
Copy link

sahil-shubham commented Mar 17, 2021

Hi @ivanoats I would like to work on this issue. This PR does not incorporate react hooks, and it would be better if I open another PR for the issue #3 rather than trying to solve the merge conflicts.

This would be my first time working on this repository, please let me know if there is anything that needs to kept in mind other than what have already been mentioned in all the contributing guidelines.

@ivanoats ivanoats closed this Mar 21, 2021
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.

4 participants