-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Migrate to css-in-js with linaria #2256
Conversation
"*.less" | ||
], | ||
"scripts": { | ||
"start": "start-storybook --quiet --no-dll -p 6006", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. A few discussion items
tabIndex={-1} | ||
onKeyDown={handleKeyDown} | ||
> | ||
{groupKey} | ||
<svg viewBox="0 0 14 8" width="14" height="8" className="rdg-caret"> | ||
<svg viewBox="0 0 14 8" width="14" height="8" className={caretClassname}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think where possible, we can move the styles in the file that use them. This way we can encapsulate styles and I think that is what CSS-in-JS libraries promote. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, in some cases the styles are intertwined and need other generated class names, so I left them alone for now.
@@ -16,10 +17,10 @@ function SummaryCell<R, SR>({ | |||
}: SummaryCellProps<R, SR>) { | |||
const { summaryFormatter: SummaryFormatter, summaryCellClass } = column; | |||
const className = clsx( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should create a helper component that can render the default styles for example
export function DefaultCell({ column, className, props }) {
className = clsx(
cellClassname,
{
[cellFrozenClassname]: column.frozen,
[cellFrozenLastClassname]: column.isLastFrozenColumn
},
className
);
return (
<div
role="gridcell"
aria-colindex={column.idx + 1}
className={className}
{...props}
>
{children}
</div>
);
}
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more stuff for react to render, not sure 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍
Docs: https://github.com/callstack/linaria
Rollup is set up so we inject the styles, that way users don't have to worry about manually importing a stylesheet.
classNameSlug
to guarantee different generated class names after each rdg version update.dist/
less
sideEffects
inpackage.json
I tried using
polished
with its babel plugin, but that didn't transpile everything away, for example:I decided to use
hsl()
instead, close enough.