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

React and Material UI dependencies updated #843

Merged
merged 8 commits into from
Aug 21, 2019
Merged

Conversation

mateusmarquezini
Copy link
Collaborator

@mateusmarquezini mateusmarquezini commented Aug 20, 2019

ISSUE #803

  • React and Material UI dependencies updated
  • Some components and props have changed on new Material UI version.

PS: Don't close the issue because the layout improvements are in progress.

const special5 = [...Array(9).keys()].map((a) => [1, a + 1]);
const special6 = [...Array(9).keys()].map((a) => [a + 1, 1]); // [1...9, 1]
const special7 = [...Array(9).keys()].map((a) => [a + 1, a + 1]); // [[1,1],...[9,9]]
const special1 = [...Array(9).keys()].map(a => [0, a + 1]);
Copy link
Owner

Choose a reason for hiding this comment

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

I know that single parameter arrow functions allow for now parenthesis but I've setup the lint rules to always enforce parenthesis for all arrow functions in order to be consistent in style across the whole repo. I think that this is super easy and quick to fix by just running npm run lintfix

Copy link
Owner

Choose a reason for hiding this comment

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

Another point. If we decided to change this lint rule, we should do that as a single distinct and separate PR and not mix it with these upgrade changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before commit all these changes and open PR, I really forgot to run this command, but I just ran npm run lintfix and nothing was changed. Maybe there are new rules with the latest eslint version we have updated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, after some attempts, it worked!

@guyellis
Copy link
Owner

@mateusmarquezini - nice - I like these changes. I made a couple of comments aimed at minimizing the number of changes in this PR. thanks.

@@ -24,8 +29,9 @@ class Keyboard extends React.Component {
}, {});

if (props.largeKeyboard) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've created 2 styles to apply here because I was getting an error before.

error: Cannot add property height, object is not extensible

So, here I only did a copy of them to buttonStyle.

Copy link
Owner

@guyellis guyellis left a comment

Choose a reason for hiding this comment

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

These changes look good to me. Feel free to merge when you're ready @mateusmarquezini

@mateusmarquezini
Copy link
Collaborator Author

These changes look good to me. Feel free to merge when you're ready @mateusmarquezini

Ok @guyellis , thanks!

@mateusmarquezini mateusmarquezini merged commit 39c665a into master Aug 21, 2019
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.

2 participants