expose variableEditorOpen to set a default value #744
Conversation
|
can this be merged? |
|
@christieb7007 looks good to me, other than the unneeded diff to |
There was a problem hiding this comment.
Thanks for your pull request! The prop name variableEditorOpen implies the parent controls whether the variable editor is open or not; however this PR only uses this prop for the initial state. Renaming it to defaultVariableEditorOpen would more accurately describe what the current implementation does.
I'd rather that we give the parent full control by using the variableEditorOpen prop in render() (falling back to state if not provided) and calling a callback onVariableEditorOpenChange to update parent state when the variable editor is toggled.
As @acao mentions, please revert changes to package.json.
|
I've updated the PR so that the |
There was a problem hiding this comment.
Thanks for updating the PR!
There are a few minor issues that this change introduces to the user experience. One that's easy to address is that if variableEditorOpen={false} and the user hovers the closed variable panel the n-resize cursor shows. We can fix that here:
graphiql/src/components/GraphiQL.js
Line 372 in 5b9012e
However the other issue that remains is the drag resizing of the variables pane. It would be a shame to lose that, but I think it will currently behave oddly because it thinks it's setting the variableEditorOpen state to false, and changing the panel height too:
graphiql/src/components/GraphiQL.js
Line 988 in 5b9012e
I think it would be good to solve these issues by making the component "controlled" so that it can send the open/close signals up to the parent component. However I think there still might be some complexity around the panel size. What do you think?
example/index.html
Outdated
| @@ -140,6 +140,7 @@ | |||
| operationName: parameters.operationName, | |||
| onEditQuery: onEditQuery, | |||
| onEditVariables: onEditVariables, | |||
| variableEditorOpen: true, | |||
There was a problem hiding this comment.
Please remove this line, I don't think we should lock the variable editor open in this example.
| variableEditorOpen: true, |
|
Apologies for the delay. |
| @@ -143,6 +143,7 @@ | |||
| operationName: parameters.operationName, | |||
| onEditQuery: onEditQuery, | |||
| onEditVariables: onEditVariables, | |||
| variableEditorOpen: true, | |||
There was a problem hiding this comment.
| variableEditorOpen: true, | |
| defaultVariableEditorOpen: true, |
| @@ -120,6 +121,9 @@ export class GraphiQL extends React.Component { | |||
| docExplorerOpen = this._storage.get('docExplorerOpen') === 'true'; | |||
| } | |||
|
|
|||
| // initial variable editor pane open | |||
| const variableEditorOpen = props.defaultVariableEditorOpen !== undefined ? props.defaultVariableEditorOpen : Boolean(variables); | |||
There was a problem hiding this comment.
(Note to self: comparing against undefined should be safe in modern browsers; I think this is okay.)
#741