-
-
Notifications
You must be signed in to change notification settings - Fork 280
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
Custom rule controls #6
Custom rule controls #6
Conversation
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 we can improve a bit more
</span> | ||
); | ||
static shouldRender(props) { | ||
return props.field === 'isDev' && props.operator === '='; |
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.
Don't think we should have this. How about using the render
itself to decide whether to render or not? Simplifies the contract of a custom control
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.
👍
@@ -24,7 +24,7 @@ export default class QueryBuilder extends React.Component { | |||
fields: React.PropTypes.array.isRequired, | |||
operators: React.PropTypes.array, | |||
combinators: React.PropTypes.array, | |||
getEditor: React.PropTypes.func, | |||
controls: React.PropTypes.object, |
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.
We should add more explicit check with React.PropTypes.Shape since we know it should have one or more of the following props: {field,operator}Selector
or valueEditor
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.
👍
|
||
if (children) { | ||
let type = children.type; | ||
if (type.shouldRender && type.shouldRender(this.props)) { |
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.
We could just use React.createElement and pass in the props. Let the custom component decide whether or not to render
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.
👍
|
||
if (children) { | ||
return ( | ||
React.cloneElement(children, |
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.
Same logic as above about not having a shouldRender
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 entirely sure what you mean here - I was hoping to check for a child element and render if it exists, otherwise fallback to the default. I can definitely remove the shouldRender
from the ValueEditor
but is there a better pattern to achieve the "fallback" effect?
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.
We can pass in the fallback element as a prop. The control can return that of it wants to use the fallback.
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.
but what if there is no custom control? i.e.
controls = {
valueEditor: null
}
<QueryBuilder controls={controls} />
There won't be a control to render the fallback, right? maybe a short snippet would help me understand better?
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.
Or are you saying setup the defaults within the QueryBuilder
component and then allow users to override it with props? I like that better actually
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.
Ya, the fallbacks will be part of QueryBuilder. Its only when we invoke the ValueEditor where we pass in the fallback as a prop.
const props = {
operator, field,
fallbackControl: (<Input type="text" />)
};
const editor = React.createElement(ValueEditor, props)
The ValueEditor can decide what to do in its render
class MyEditor extends React.Component {
render() {
// render the custom editor or use this.props.fallbackControl as the return value
}
}
HTH.
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.
Also if there is no customControl provided, we use the fallback baked into QueryBuilder
b585dc4
to
c1a4b4b
Compare
made a lot of changes to see if it fits better now. Also, do you want me to run the distribution build as well to check in the build artifacts along with this? or does that come as a separate PR? |
not sure if you saw the latest updates - it uses the |
Little swamped right now. Will take a look later tonight. Might have missed it. Thanks for checking in Thanks,
|
hi just checking in to see if you had a look yet. no rush, just seeing if there is anything else I could do |
Looks good to me overall. Will merge. |
I think we also need to improve the code coverage in our unit tests. I was thinking of using Jest. What do you think? |
@maniax89 BTW, I think now would be a good time to start a I'll make a new NPM release once you have it 🍸 |
Yes I can start a CHANGELOG.md with a list of feature additions. I guess I will add the tag as well. Since you were OK with the changes I made - I was going to continue doing the same with the buttons in the same fashion so all the components are customizable. In terms of Jest, I haven't used it before but sounds fine. I think it would be good to get a travis.yml file in here so that the tests run with each PR and then merging is dependent on passing those tests. I think it is as easy as logging into travis https://docs.travis-ci.com/user/getting-started/#Step-one%3A-Sign-in then adding this file to the repo:
|
Documentation for RQB 4.2.0
Adds in the ability to customize the components used for the controls for the and elements in the element.
Can be utilized similar to how the
getEditor
command worked before:Addresses #2