CSS-in-JS Attempt v2 #73
Conversation
src/index.js
Outdated
|
|
||
| this.index = index = index + 1; | ||
| } | ||
| const Cell = styled.div` |
There was a problem hiding this comment.
@horacioh I think these cells should still have the classNames they had before so people who do use css can easily select them. I think the tests should include those selectors as well, as they did before to be clear about what is being selected rather than just 'span's and 'div's.
There was a problem hiding this comment.
yeah this is something I was thinking before, we can leave the classes, but with no extra css for them, just for users to override it.
another option i thought was to have this refactor as a breaking change. but I dunno about how people will react to this...
I'm OK with both options!
about the tests, I think targeting the actual components is clear enough. what do you think is different from this compared to css classes?
There was a problem hiding this comment.
Let's keep the classes. Will comment inline in test file to show
| @@ -6,7 +6,9 @@ | |||
| "bugs": { | |||
| "url": "https://github.com/henrybuilt/react-sticky-table/issues" | |||
| }, | |||
| "dependencies": {}, | |||
| "dependencies": { | |||
| "styled-components": "^4.3.2" | |||
There was a problem hiding this comment.
what is the difference between emotion and styled-components, or are they the same? My understanding was that they were different, but am seeing a mix here
There was a problem hiding this comment.
they are both solving the same problem: both are libraries to write CSS in JS. emotions used to be faster, but now seems styled components to be more performant.
I changed it to styled components on this version.
|
@horacioh I haven't actually checked this out on my computer yet, but looks good other than the comments I left |
babel.config.js
Outdated
| @@ -12,7 +12,7 @@ module.exports = { | |||
| } | |||
| ] | |||
| ], | |||
| plugins: ['@babel/plugin-proposal-class-properties'], | |||
| plugins: ['@babel/plugin-proposal-class-properties', 'emotion'], | |||
There was a problem hiding this comment.
@horacioh seems like this should be removed based on what you said about styled components
There was a problem hiding this comment.
thanks for the review!
babel.config.js
Outdated
| @@ -25,7 +25,7 @@ module.exports = { | |||
| ], | |||
| '@babel/preset-react' | |||
| ], | |||
| plugins: ['@babel/plugin-proposal-class-properties'] | |||
| plugins: ['@babel/plugin-proposal-class-properties', 'emotion'] | |||
| @@ -17,6 +17,6 @@ describe('Cell', () => { | |||
| </Cell> | |||
| ); | |||
|
|
|||
| expect(cell.find('.sticky-table-cell span')).to.have.length(2); | |||
| expect(cell.find(Cell).find('span')).to.have.length(2); | |||
There was a problem hiding this comment.
@horacioh nevermind about tests - this was the only one I noticed, and it's clear the way you have it
|
@horacioh Please check the storybook by running It appears the styles are not working - I'm not sure why. Also getting this error which would be great if you could fix: The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type". |
|
@maxhudson can you share a screenshot of what you see on the storybooks? I just run it and everything seems to be ok..
|
|
@maxhudson the ":first-child" error seems to be some storybook error |
|
|
|
@horacioh after looking at this more, I'm excited about it as it cleans things up quite a bit! Just gotta figure out what's up with storybook, which I'm not sure how to debug as there's no problem on your end and on my end I'm not seeing any reason for the lack of functionality |
|
Can you also resolve conflict with index.js? |
|
@maxhudson sure thing! will do it later today :) |
|
The commit I just pushed fixed my storybook issue |
|
@horacioh once that conflict is resolved we can merge! |
|
@maxhudson hey! 👋 finally i got time to fix the conflict. I also delete duplicate css from the storybook demo. ti had an extra border that was not correctly being positioned. hope now is ready to merge! :) |
|
@horacioh merged! Thanks for all the help :) |
closes #62
index.jsnow)styled-componentson this one. seems to be a more performant solution all well documented and adopted.Math.mincall onstickyHeaderCount&stickyColumnCount. that was causing to just return 1 everytime. maybe if you want more than 1 column/row fixed this is not helpful.PLEASE GIVE SOME FEEDBACK!!
this is IMHO a good starting point to start migrating the code. also I can after this PR being merged, migrate the code to Typescript :)