Fix fatal error on time series#9
Fix fatal error on time series#9CedricProfessionnel wants to merge 4 commits intoUniversalDataTool:masterfrom
Conversation
seveibar
left a comment
seveibar
left a comment
There was a problem hiding this comment.
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 |
|
Do you have the warning? camelCase props are really common |
|
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 |
|
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 |
@seveibar you asked for the warning this is it. I will check for still using the camelcase and this is the reason why is asking this : |
|
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 ... |
|
@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) |
|
good find |
seveibar
left a comment
seveibar
left a comment
There was a problem hiding this comment.
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 fetchgit checkout origin/mastergit 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 commitgit checkout -- .to remove all the unstaged changes that weren't necessary
src/components/Timeline/index.js
Outdated
| key={timeTextIndex} | ||
| x={(timeTextIndex / timeTextCount) * width} | ||
| faded={timeTextTimes[timeTextIndex] < 0} | ||
| faded={(timeTextTimes[timeTextIndex] < 0).toString()} |
There was a problem hiding this comment.
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, | ||
| })) |
There was a problem hiding this comment.
This shouldn't be defined inside of the component
| onClick={onClick} | ||
| width={width} | ||
| color={color} | ||
| active={active.toString()} |
There was a problem hiding this comment.
active should not be a string
2 participants
Prevent the fatal error on Next
and remove some warnings