New implementation of code editor#411
Conversation
* Implemented CodeEditor using `react-codemirror2`. Closes plotly#401 Closes plotly#360
* Uses react-resizable Closes plotly#311
* Removed component CodeEditorField. * Removed dependency react-codemirror.
n-riesco
force-pushed
the
editor/use-react-codemirror2
branch
from
586f28e to
dc37d26
Compare
* Added a margin between the COdeEditor and SQLTable. * Removed margin between the tab headers (Table, Chart, Export) and the corresponding tab panels. * Reduce initial size of CodeEditor from 250px to 140px. * Increase initial size of SQLTable from 200px to 300px. * Updated the styles for SQLTable so that it uses the same color scheme as CodeEditor. * Added a link to toggle the row filters in SQLTable.
n-riesco
force-pushed
the
editor/use-react-codemirror2
branch
from
dc37d26 to
c4f215d
Compare
|
@jackparmer @shannonlal I've updated the PR with a few, small fixes I want to merge before we release Falcon v2.6:
Please, let me knwo what you think of the UI changes.
|
shannonlal
left a comment
shannonlal
left a comment
There was a problem hiding this comment.
Overall looks really good. My comments are all minor and don't want to block the release. Good job💃
| const height = wrapperElement.clientHeight; | ||
| const width = wrapperElement.clientWidth; | ||
|
|
||
| const minConstraints = [width, 74]; |
There was a problem hiding this comment.
Maybe put 74 as a constant at top with what it represents. const MIN_WIDTH_CONSTRAINTS = 74
| static computeAutocompleteTables(props) { | ||
| const schemaRequest = props.schemaRequest || {}; | ||
|
|
||
| if (schemaRequest.status !== 200 || !schemaRequest.content || !schemaRequest.content.rows) { |
There was a problem hiding this comment.
Should we be moving connection status to a centralized location or atleast have this as a const? Minor thing
There was a problem hiding this comment.
No, let's not do that. There is no gain in having STATUS_OK_200 vs 200.
| maxConstraints | ||
| } = this.state; | ||
|
|
||
| const mode = { |
There was a problem hiding this comment.
If we add other dialects should the be added here? Should we update the NEW_CONNECTION doc?
|
|
||
| minWidth={740} | ||
| minHeight={200} | ||
| minHeight={300} |
There was a problem hiding this comment.
Minor thing. Do these variables belong in const?
There was a problem hiding this comment.
No, I wouldn't. My personal criteria for defining a const is:
- if the constant is used only once or twice, I only define a
constif it documents its meaning; e.g. inminHeight={300}, I don't define a constant, because it's clear that 300 is the minimum height passed to ReactDataGrid. - if the constant is used twice or more times, then I define a constant, because it's easier to update
const MY_CONST = 300;in one place than 300 in multiple places.
|
Added minor comments that you can review if you have time 💃 |
* Document how to set the codemirror mode used in the code editor.
2 participants
react-codemirrorwithreact-codemirror2react-resizableto implement a resize handleCodeEditor,SQLTableand tab list)Closes #401
Closes #360
Closes #311
Closes #412