-
-
Notifications
You must be signed in to change notification settings - Fork 492
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
feat: Color scheme customization #305
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 neat! Added some points to discuss, but I think we can quickly move to docs (keep it simple) & tests
// Sets blockContent class | ||
blockContent.className = styles.blockContent; | ||
// Add custom HTML attributes | ||
const blockContentDOMAttributes = |
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.
why not blockContentDOMAttributes.blockContent.class?
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.
For each of the DOM element types, you can add custom attributes, not just a custom class
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.
Ok, you could do this:
constr { class, ...otherAttributes} = this.options.domAttributes?.blockContent || {};
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.
Actually this also doesn't really work since class
is a reserved word for class definitions
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.
Check. We could decide to rename class
to className
, that's also inline with how React does 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.
Not sure if it's a good idea tbh, since we're not only applying the class to React components. Basic nodes like blockContainer
, blockGroup
, etc, are defined outside of React, so I think it's better to keep HTML naming conventions for this rather than React ones
packages/react/src/SharedComponents/ColorPicker/components/ColorIcon.tsx
Outdated
Show resolved
Hide resolved
// Sets blockContent class | ||
blockContent.className = styles.blockContent; | ||
// Add custom HTML attributes | ||
const blockContentDOMAttributes = |
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.
Ok, you could do this:
constr { class, ...otherAttributes} = this.options.domAttributes?.blockContent || {};
packages/react/src/SharedComponents/ColorPicker/components/ColorIcon.tsx
Outdated
Show resolved
Hide resolved
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.
some more comments
|
||
Take a look at how this is done in the demo below: | ||
|
||
::: sandbox {template=react-ts} |
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.
For sake of simplicity, I would just make one "red theme" and not a red light / dark theme
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 it's nice to have both since it both shows off the auto switching from browser settings and saves us having to explain how to pass in a light & dark theme instead of just a single theme. Theming is also not really complicated compared to a lot of the other stuff, since 90% of it is defining colors, so imo we shouldn't run into the issue of people getting overloaded with information.
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!
This PR overhauls how users can style the BlockNote editor. This is done in 3 ways:
There are still a few points of contention:
One of the main limitations is the extended theming customizability. I think most people are interested in just changing colors and fonts, for which we have a good solution, but overriding CSS could probably use some improvements for the future.
Another limitation is that the HTML classes set on elements cannot be dynamically changed atm. I can see this being a blocker for some as setting
hidden
,locked
, orselected
classes is a pretty common use case. However, the implementation is actually pretty complex so not sure it's worth our time atm.