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

Fix fatal error on time series #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

CedricProfessionnel
Copy link
Contributor

@CedricProfessionnel CedricProfessionnel commented Jan 14, 2021

Prevent the fatal error on Next
and remove some warnings

Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

was there a reason to rename themeColors to themecolors? Other than that it looks good but themeColors (the original) was correct camelcasing

@CedricProfessionnel
Copy link
Contributor Author

was there a reason to rename themeColors to themecolors? Other than that it looks good but themeColors (the original) was correct camelcasing

There was a warning in the console who ask for the change

@seveibar
Copy link
Contributor

Do you have the warning? camelCase props are really common

@seveibar
Copy link
Contributor

I appreciate the fixing of the linting errors btw, but it would just be weird if we had one variable specifically excluded from the camelCase convention

@seveibar
Copy link
Contributor

The only thing I can think of is maybe we were overriding something that Material UI had defined, in which case, the solution would be to either 1) pick a different camelCase name or 2) use whatever approach material ui recommends

I won't bikeshed on this too long, but I'm a bit curious why the linter would error here

@CedricProfessionnel
Copy link
Contributor Author

CedricProfessionnel commented Jan 14, 2021

Do you have the warning? camelCase props are really common

@seveibar you asked for the warning this is it. I will check for still using the camelcase
image

and this is the reason why is asking this :
React JSX transform uses the upper vs. lower case convention to distinguish between user-defined components and DOM tags.

@seveibar
Copy link
Contributor

Thanks Cedric, this is a strange issue, so what's happening is React doesn't like it when you give unknown props to dom elements. The reason that's happening is bevaused of styled components i think. Is there an alternate solution where we can avoid passing that prop to a dom element?

@CedricProfessionnel
Copy link
Contributor Author

CedricProfessionnel commented Jan 14, 2021

Thanks Cedric, this is a strange issue, so what's happening is React doesn't like it when you give unknown props to dom elements. The reason that's happening is bevaused of styled components i think. Is there an alternate solution where we can avoid passing that prop to a dom element?

@seveibar styled-components/styled-components#2093 It's possible but our nomenclature will need to change. So we return to the starting point ...

@seveibar
Copy link
Contributor

@CedricProfessionnel I think the transient prop is the correct solution, because we shouldn't be passing themecolors nor themeColors to the dom element (since it doesn't know how to handle them)

@seveibar
Copy link
Contributor

good find

Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

Hey Cedric, I think you should git cherry-pick the fix or copy the important lines over, if you look at the files changed there were a lot of unnecessary changes and little bugs introduced.

Here's what I normally do when this happens to me:

  • git fetch
  • git checkout origin/master
  • git merge --squash --no-commit MY_BRANCH_NAME
  • Now I have a bunch of unstaged changes. I go through each change in an IDE like VS Code, Atom or if you're comfortable with git chunking git add -i. Any change that doesn't seem necessary I don't add
  • git commit
  • git checkout -- . to remove all the unstaged changes that weren't necessary

onClick={rootAudioElm ? onClickTimeline : undefined}
>
{range(timeTextCount).map((timeTextIndex) => (
<TimeText
key={timeTextIndex}
x={(timeTextIndex / timeTextCount) * width}
faded={timeTextTimes[timeTextIndex] < 0}
faded={(timeTextTimes[timeTextIndex] < 0).toString()}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a boolean not a string, .toString() will make this always truthy now

return <Container themeColors={themeColors}>{children}</Container>
const Container = styled("div")(() => ({
backgroundColor: themeColors.bg,
}))
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be defined inside of the component

onClick={onClick}
width={width}
color={color}
active={active.toString()}
Copy link
Contributor

Choose a reason for hiding this comment

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

active should not be a string

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