-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add ClassNames support to the customStyleMap #2164
base: main
Are you sure you want to change the base?
Add ClassNames support to the customStyleMap #2164
Conversation
// minification by themselves. This is handy when used libraries like `jss` | ||
// or similar | ||
{className: classNames.join(' ').trim()} | ||
: {}; |
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.
Setting classes to null or undefined would already create a dom node without an empty class attribute.
https://codesandbox.io/s/upbeat-yalow-uwkoh
Spreading here is just unnecessary and not good for perf.
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.
thanks for looking into this
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.
changed
@newsiberian Do you plan to revisit this? I would love to have this feature. |
What for? I did what @fabiomcosta asked. We are successfully use this for about the year |
@fabiomcosta Oh my bad, I somehow skipped that. But thx for the response. |
@newsiberian Sorry, even with that information i cannot figure out how to add a custom css class to a existing style like "BOLD". Here is my
Could you help me out? |
we use something like this: export const getStyleMap = (theme, classes) => ({
[FONT_COLOR_ONE]: classes.fontColorOne,
[FONT_COLOR_TWO]: classes.fontColorTwo,
[FONT_COLOR_THREE]: classes.fontColorThree,
[FONT_COLOR_FOUR]: classes.fontColorFour,
[FONT_COLOR_FIVE]: classes.fontColorFive,
[FONT_SIZE_14]: classes.fontSize14,
[FONT_SIZE_16]: classes.fontSize16,
[FONT_SIZE_18]: classes.fontSize18,
[FONT_SIZE_20]: classes.fontSize20,
[FONT_SIZE_24]: classes.fontSize24,
[FONT_SIZE_28]: classes.fontSize28,
[FONT_SIZE_32]: classes.fontSize32,
[FONT_SIZE_36]: classes.fontSize36,
[FONT_SIZE_42]: classes.fontSize42,
[FONT_SIZE_48]: classes.fontSize48,
[FONT_SIZE_54]: classes.fontSize54,
[FONT_SIZE_60]: classes.fontSize60,
[VERTICAL_ALIGN_TOP]: {
verticalAlign: 'super',
},
[VERTICAL_ALIGN_BOTTOM]: {
verticalAlign: 'sub',
},
}); then, in component: // theme / classes from material-ui solution
const memoizedStyleMap = useMemo(() => getStyleMap(theme, classes), [
theme,
classes,
]);
// then
return <Editor ...props styleMap={memoizedStyleMap} /> so, it is similar like you do |
@newsiberian So wait, where are those classes, are they css in js or actual css?
|
Is this getting merged? @fabiomcosta Thanks :) |
@DaniGuardiola I'm actually not a maintainer of this project, but contribute here and there. |
let styleObj = styleSet.reduce((map, styleName) => { | ||
const mergedStyles = {}; | ||
const style = customStyleMap[styleName]; | ||
let style; | ||
if (typeof customStyleMap[styleName] === 'string') { |
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.
Maybe would be better to add a new prop customCssClassMap
. This way you don't need this check.
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 did this approach here #2930
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.
Your version looks cleaner. I like it)
Summary
This PR makes it possible to pass classNames (strings) instead of styles (objects) to the
customStyleMap
Resolves #957, resolves #1368, resolves #1639
and partly related with #342, #521
If you will accept this PR, please, let me know which part of documentation should be updated and how
Test Plan
This changes can be tested via
Color
example which was slightly modified