-
Notifications
You must be signed in to change notification settings - Fork 33
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
[WIP] Add blockTypesData
for additional props.
#141
base: master
Are you sure you want to change the base?
Conversation
Specify (optional) blockType-specific data to pass as props on render. # Motivation Imagine a component that selects from a list: ``` { id: 'animals', label: 'Animals', component: Animals } ``` You use 'animals' in many places. But then a new component request comes in: it should be similar to 'animals', but filter results to show only warm-blooded animals. Now you could create a brand new blockType, and that might be appropriate in certain situations. But in many cases, it would be simpler to reuse component logic with props. # Proposal Use a new bootstrap options key, `blockTypesData`, as a map of `blockType.id` keys. The value for each key is an object, which becomes a part of the `BlockType` as `data` and spread as props for the component. I'm open to approaching this in other ways. This is a first pass that I hope helps to communicate the end goal.
Codecov Report
@@ Coverage Diff @@
## master #141 +/- ##
==========================================
- Coverage 93.49% 93.33% -0.17%
==========================================
Files 33 33
Lines 246 255 +9
Branches 28 30 +2
==========================================
+ Hits 230 238 +8
- Misses 14 15 +1
Partials 2 2
Continue to review full report at Codecov.
|
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 the idea. I'll defer to someone who knows more about CK internals to say if this is a good implementation for it.
@@ -28,6 +28,8 @@ export default class YouTube extends React.Component { | |||
render() { | |||
const { baseUrl, content } = this.props | |||
|
|||
console.log(this.props.name) |
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.
for sure! left this here just for demonstration purposes.
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.
This looks good to me. We should add documentation for it.
What about calling this blockOptions
? Additionally, we could expose the ability to add a static to each component to provide defaults, like:
class YouTubeBlock extends React.Component {
static options = {
name: "User"
}
}
let editor = new ColonelKurtz({
// ...
blockTypeOptions: {
youtube: { name: 'Chris' }
}
})
We'd also pass the value as options
to the block.
Thanks, @nhunzaker. And good suggestion on the name |
@@ -74,6 +79,7 @@ export default class Block extends React.PureComponent { | |||
<Component | |||
ref={el => (this.block = el)} | |||
{...block} | |||
{...options} |
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'm worried this will cause bugs where an option field could clear important block properties. What if this were passed as options={options}
?
Is there a reason for placing this config as an option in the call to instantiate CK? As an alternative, each item in the Both routes seem reasonable to me but specifying options for a particular block type within the block type definition kind of makes more sense to me. ./loosely-held-opinion |
A few additional thoughts based on a conversation with @cwmanning:
I'm thinking it would be useful to have a few layers of configuration with the values specified directly on the input taking precedence. |
Specify (optional) blockType-specific data to pass
as props on render.
Motivation
Imagine a component that selects from a list:
You use 'animals' in many places. But then a new
component request comes in: it should be similar
to 'animals', but filter results to show only
warm-blooded animals.
Now you could create a brand new
blockType, and that might be appropriate in
certain situations. But in many cases, it would be
simpler to reuse component logic with props.
Proposal
Use a new bootstrap options key,
blockTypesData
,as a map of
blockType.id
keys. The value foreach key is an object, which becomes a part of the
BlockType
asdata
and spread as props for thecomponent.
I'm open to approaching this in other ways. This
is a first pass that I hope helps communicate
the end goal.