This repository was archived by the owner on Aug 29, 2025. It is now read-only.

New implementation of code editor - #411

Merged
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2
Apr 11, 2018
Merged

New implementation of code editor#411
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2

Conversation

@n-riesco

@n-riescon-riesco commented Apr 6, 2018

Copy link
Copy Markdown
Contributor
  • replaced react-codemirror with react-codemirror2
  • used react-resizable to implement a resize handle
  • improved CSS styles (CodeEditor, SQLTable and tab list)
  • TODO: add jest test?

Closes#401
Closes#360
Closes#311
Closes#412

peek 2018-04-06 23-32

* Implemented CodeEditor using `react-codemirror2`.
Closesplotly#401Closesplotly#360
* Uses react-resizable
Closesplotly#311
* Removed component CodeEditorField.
* Removed dependency react-codemirror.
@n-riesco
n-riescoforce-pushed the editor/use-react-codemirror2 branch from 586f28e to dc37d26CompareApril 10, 2018 16:51
* 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
n-riescoforce-pushed the editor/use-react-codemirror2 branch from dc37d26 to c4f215dCompareApril 10, 2018 16:58
@n-riesco

n-riesco commented Apr 10, 2018

Copy link
Copy Markdown
ContributorAuthor

@jackparmer@shannonlal I've updated the PR with a few, small fixes I want to merge before we release Falcon v2.6:

  • I've made the initial size of the code editor smaller and the table view accordingly larger.
  • the table view now uses the same color theme as the code editor.
  • I've changed the way to toggle the row filter (I've a added a link similar to the one used to toggle the code editor).
  • I've updated ibm_dbto v2.3 (no need for my fork any longer!!!)

Please, let me knwo what you think of the UI changes.

peek 2018-04-10 18-29

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be moving connection status to a centralized location or atleast have this as a const? Minor thing

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's not do that. There is no gain in having STATUS_OK_200 vs 200.

maxConstraints
} = this.state;

const mode = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add other dialects should the be added here? Should we update the NEW_CONNECTION doc?


minWidth={740}
minHeight={200}
minHeight={300}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing. Do these variables belong in const?

@n-riescon-riescoApr 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 const if it documents its meaning; e.g. in minHeight={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.

@shannonlal

Copy link
Copy Markdown
Contributor

Added minor comments that you can review if you have time 💃

@n-riesco
n-riesco merged commit 96dfe32 into plotly:masterApr 11, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@n-riesco@shannonlal
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

New implementation of code editor - #411

Merged
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2
Apr 11, 2018
Merged

New implementation of code editor#411
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2

Conversation

@n-riesco

@n-riescon-riesco commented Apr 6, 2018

Copy link
Copy Markdown
Contributor
  • replaced react-codemirror with react-codemirror2
  • used react-resizable to implement a resize handle
  • improved CSS styles (CodeEditor, SQLTable and tab list)
  • TODO: add jest test?

Closes#401
Closes#360
Closes#311
Closes#412

peek 2018-04-06 23-32

* Implemented CodeEditor using `react-codemirror2`.
Closesplotly#401Closesplotly#360
* Uses react-resizable
Closesplotly#311
* Removed component CodeEditorField.
* Removed dependency react-codemirror.
@n-riesco
n-riescoforce-pushed the editor/use-react-codemirror2 branch from 586f28e to dc37d26CompareApril 10, 2018 16:51
* 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
n-riescoforce-pushed the editor/use-react-codemirror2 branch from dc37d26 to c4f215dCompareApril 10, 2018 16:58
@n-riesco

n-riesco commented Apr 10, 2018

Copy link
Copy Markdown
ContributorAuthor

@jackparmer@shannonlal I've updated the PR with a few, small fixes I want to merge before we release Falcon v2.6:

  • I've made the initial size of the code editor smaller and the table view accordingly larger.
  • the table view now uses the same color theme as the code editor.
  • I've changed the way to toggle the row filter (I've a added a link similar to the one used to toggle the code editor).
  • I've updated ibm_dbto v2.3 (no need for my fork any longer!!!)

Please, let me knwo what you think of the UI changes.

peek 2018-04-10 18-29

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be moving connection status to a centralized location or atleast have this as a const? Minor thing

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's not do that. There is no gain in having STATUS_OK_200 vs 200.

maxConstraints
} = this.state;

const mode = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add other dialects should the be added here? Should we update the NEW_CONNECTION doc?


minWidth={740}
minHeight={200}
minHeight={300}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing. Do these variables belong in const?

@n-riescon-riescoApr 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 const if it documents its meaning; e.g. in minHeight={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.

@shannonlal

Copy link
Copy Markdown
Contributor

Added minor comments that you can review if you have time 💃

@n-riesco
n-riesco merged commit 96dfe32 into plotly:masterApr 11, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@n-riesco@shannonlal
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

New implementation of code editor - #411

Merged
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2
Apr 11, 2018
Merged

New implementation of code editor#411
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2

Conversation

@n-riesco

@n-riescon-riesco commented Apr 6, 2018

Copy link
Copy Markdown
Contributor
  • replaced react-codemirror with react-codemirror2
  • used react-resizable to implement a resize handle
  • improved CSS styles (CodeEditor, SQLTable and tab list)
  • TODO: add jest test?

Closes#401
Closes#360
Closes#311
Closes#412

peek 2018-04-06 23-32

* Implemented CodeEditor using `react-codemirror2`.
Closesplotly#401Closesplotly#360
* Uses react-resizable
Closesplotly#311
* Removed component CodeEditorField.
* Removed dependency react-codemirror.
@n-riesco
n-riescoforce-pushed the editor/use-react-codemirror2 branch from 586f28e to dc37d26CompareApril 10, 2018 16:51
* 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
n-riescoforce-pushed the editor/use-react-codemirror2 branch from dc37d26 to c4f215dCompareApril 10, 2018 16:58
@n-riesco

n-riesco commented Apr 10, 2018

Copy link
Copy Markdown
ContributorAuthor

@jackparmer@shannonlal I've updated the PR with a few, small fixes I want to merge before we release Falcon v2.6:

  • I've made the initial size of the code editor smaller and the table view accordingly larger.
  • the table view now uses the same color theme as the code editor.
  • I've changed the way to toggle the row filter (I've a added a link similar to the one used to toggle the code editor).
  • I've updated ibm_dbto v2.3 (no need for my fork any longer!!!)

Please, let me knwo what you think of the UI changes.

peek 2018-04-10 18-29

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be moving connection status to a centralized location or atleast have this as a const? Minor thing

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's not do that. There is no gain in having STATUS_OK_200 vs 200.

maxConstraints
} = this.state;

const mode = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add other dialects should the be added here? Should we update the NEW_CONNECTION doc?


minWidth={740}
minHeight={200}
minHeight={300}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing. Do these variables belong in const?

@n-riescon-riescoApr 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 const if it documents its meaning; e.g. in minHeight={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.

@shannonlal

Copy link
Copy Markdown
Contributor

Added minor comments that you can review if you have time 💃

@n-riesco
n-riesco merged commit 96dfe32 into plotly:masterApr 11, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@n-riesco@shannonlal
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

New implementation of code editor - #411

Merged
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2
Apr 11, 2018
Merged

New implementation of code editor#411
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2

Conversation

@n-riesco

@n-riescon-riesco commented Apr 6, 2018

Copy link
Copy Markdown
Contributor
  • replaced react-codemirror with react-codemirror2
  • used react-resizable to implement a resize handle
  • improved CSS styles (CodeEditor, SQLTable and tab list)
  • TODO: add jest test?

Closes#401
Closes#360
Closes#311
Closes#412

peek 2018-04-06 23-32

* Implemented CodeEditor using `react-codemirror2`.
Closesplotly#401Closesplotly#360
* Uses react-resizable
Closesplotly#311
* Removed component CodeEditorField.
* Removed dependency react-codemirror.
@n-riesco
n-riescoforce-pushed the editor/use-react-codemirror2 branch from 586f28e to dc37d26CompareApril 10, 2018 16:51
* 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
n-riescoforce-pushed the editor/use-react-codemirror2 branch from dc37d26 to c4f215dCompareApril 10, 2018 16:58
@n-riesco

n-riesco commented Apr 10, 2018

Copy link
Copy Markdown
ContributorAuthor

@jackparmer@shannonlal I've updated the PR with a few, small fixes I want to merge before we release Falcon v2.6:

  • I've made the initial size of the code editor smaller and the table view accordingly larger.
  • the table view now uses the same color theme as the code editor.
  • I've changed the way to toggle the row filter (I've a added a link similar to the one used to toggle the code editor).
  • I've updated ibm_dbto v2.3 (no need for my fork any longer!!!)

Please, let me knwo what you think of the UI changes.

peek 2018-04-10 18-29

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be moving connection status to a centralized location or atleast have this as a const? Minor thing

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's not do that. There is no gain in having STATUS_OK_200 vs 200.

maxConstraints
} = this.state;

const mode = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add other dialects should the be added here? Should we update the NEW_CONNECTION doc?


minWidth={740}
minHeight={200}
minHeight={300}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing. Do these variables belong in const?

@n-riescon-riescoApr 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 const if it documents its meaning; e.g. in minHeight={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.

@shannonlal

Copy link
Copy Markdown
Contributor

Added minor comments that you can review if you have time 💃

@n-riesco
n-riesco merged commit 96dfe32 into plotly:masterApr 11, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@n-riesco@shannonlal
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

New implementation of code editor - #411

Merged
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2
Apr 11, 2018
Merged

New implementation of code editor#411
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2

Conversation

@n-riesco

@n-riescon-riesco commented Apr 6, 2018

Copy link
Copy Markdown
Contributor
  • replaced react-codemirror with react-codemirror2
  • used react-resizable to implement a resize handle
  • improved CSS styles (CodeEditor, SQLTable and tab list)
  • TODO: add jest test?

Closes#401
Closes#360
Closes#311
Closes#412

peek 2018-04-06 23-32

* Implemented CodeEditor using `react-codemirror2`.
Closesplotly#401Closesplotly#360
* Uses react-resizable
Closesplotly#311
* Removed component CodeEditorField.
* Removed dependency react-codemirror.
@n-riesco
n-riescoforce-pushed the editor/use-react-codemirror2 branch from 586f28e to dc37d26CompareApril 10, 2018 16:51
* 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
n-riescoforce-pushed the editor/use-react-codemirror2 branch from dc37d26 to c4f215dCompareApril 10, 2018 16:58
@n-riesco

n-riesco commented Apr 10, 2018

Copy link
Copy Markdown
ContributorAuthor

@jackparmer@shannonlal I've updated the PR with a few, small fixes I want to merge before we release Falcon v2.6:

  • I've made the initial size of the code editor smaller and the table view accordingly larger.
  • the table view now uses the same color theme as the code editor.
  • I've changed the way to toggle the row filter (I've a added a link similar to the one used to toggle the code editor).
  • I've updated ibm_dbto v2.3 (no need for my fork any longer!!!)

Please, let me knwo what you think of the UI changes.

peek 2018-04-10 18-29

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be moving connection status to a centralized location or atleast have this as a const? Minor thing

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's not do that. There is no gain in having STATUS_OK_200 vs 200.

maxConstraints
} = this.state;

const mode = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add other dialects should the be added here? Should we update the NEW_CONNECTION doc?


minWidth={740}
minHeight={200}
minHeight={300}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing. Do these variables belong in const?

@n-riescon-riescoApr 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 const if it documents its meaning; e.g. in minHeight={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.

@shannonlal

Copy link
Copy Markdown
Contributor

Added minor comments that you can review if you have time 💃

@n-riesco
n-riesco merged commit 96dfe32 into plotly:masterApr 11, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@n-riesco@shannonlal
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

New implementation of code editor - #411

Merged
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2
Apr 11, 2018
Merged

New implementation of code editor#411
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2

Conversation

@n-riesco

@n-riescon-riesco commented Apr 6, 2018

Copy link
Copy Markdown
Contributor
  • replaced react-codemirror with react-codemirror2
  • used react-resizable to implement a resize handle
  • improved CSS styles (CodeEditor, SQLTable and tab list)
  • TODO: add jest test?

Closes#401
Closes#360
Closes#311
Closes#412

peek 2018-04-06 23-32

* Implemented CodeEditor using `react-codemirror2`.
Closesplotly#401Closesplotly#360
* Uses react-resizable
Closesplotly#311
* Removed component CodeEditorField.
* Removed dependency react-codemirror.
@n-riesco
n-riescoforce-pushed the editor/use-react-codemirror2 branch from 586f28e to dc37d26CompareApril 10, 2018 16:51
* 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
n-riescoforce-pushed the editor/use-react-codemirror2 branch from dc37d26 to c4f215dCompareApril 10, 2018 16:58
@n-riesco

n-riesco commented Apr 10, 2018

Copy link
Copy Markdown
ContributorAuthor

@jackparmer@shannonlal I've updated the PR with a few, small fixes I want to merge before we release Falcon v2.6:

  • I've made the initial size of the code editor smaller and the table view accordingly larger.
  • the table view now uses the same color theme as the code editor.
  • I've changed the way to toggle the row filter (I've a added a link similar to the one used to toggle the code editor).
  • I've updated ibm_dbto v2.3 (no need for my fork any longer!!!)

Please, let me knwo what you think of the UI changes.

peek 2018-04-10 18-29

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be moving connection status to a centralized location or atleast have this as a const? Minor thing

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's not do that. There is no gain in having STATUS_OK_200 vs 200.

maxConstraints
} = this.state;

const mode = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add other dialects should the be added here? Should we update the NEW_CONNECTION doc?


minWidth={740}
minHeight={200}
minHeight={300}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing. Do these variables belong in const?

@n-riescon-riescoApr 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 const if it documents its meaning; e.g. in minHeight={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.

@shannonlal

Copy link
Copy Markdown
Contributor

Added minor comments that you can review if you have time 💃

@n-riesco
n-riesco merged commit 96dfe32 into plotly:masterApr 11, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@n-riesco@shannonlal
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

New implementation of code editor - #411

Merged
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2
Apr 11, 2018
Merged

New implementation of code editor#411
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2

Conversation

@n-riesco

@n-riescon-riesco commented Apr 6, 2018

Copy link
Copy Markdown
Contributor
  • replaced react-codemirror with react-codemirror2
  • used react-resizable to implement a resize handle
  • improved CSS styles (CodeEditor, SQLTable and tab list)
  • TODO: add jest test?

Closes#401
Closes#360
Closes#311
Closes#412

peek 2018-04-06 23-32

* Implemented CodeEditor using `react-codemirror2`.
Closesplotly#401Closesplotly#360
* Uses react-resizable
Closesplotly#311
* Removed component CodeEditorField.
* Removed dependency react-codemirror.
@n-riesco
n-riescoforce-pushed the editor/use-react-codemirror2 branch from 586f28e to dc37d26CompareApril 10, 2018 16:51
* 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
n-riescoforce-pushed the editor/use-react-codemirror2 branch from dc37d26 to c4f215dCompareApril 10, 2018 16:58
@n-riesco

n-riesco commented Apr 10, 2018

Copy link
Copy Markdown
ContributorAuthor

@jackparmer@shannonlal I've updated the PR with a few, small fixes I want to merge before we release Falcon v2.6:

  • I've made the initial size of the code editor smaller and the table view accordingly larger.
  • the table view now uses the same color theme as the code editor.
  • I've changed the way to toggle the row filter (I've a added a link similar to the one used to toggle the code editor).
  • I've updated ibm_dbto v2.3 (no need for my fork any longer!!!)

Please, let me knwo what you think of the UI changes.

peek 2018-04-10 18-29

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be moving connection status to a centralized location or atleast have this as a const? Minor thing

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's not do that. There is no gain in having STATUS_OK_200 vs 200.

maxConstraints
} = this.state;

const mode = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add other dialects should the be added here? Should we update the NEW_CONNECTION doc?


minWidth={740}
minHeight={200}
minHeight={300}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing. Do these variables belong in const?

@n-riescon-riescoApr 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 const if it documents its meaning; e.g. in minHeight={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.

@shannonlal

Copy link
Copy Markdown
Contributor

Added minor comments that you can review if you have time 💃

@n-riesco
n-riesco merged commit 96dfe32 into plotly:masterApr 11, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@n-riesco@shannonlal
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

New implementation of code editor - #411

Merged
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2
Apr 11, 2018
Merged

New implementation of code editor#411
n-riesco merged 10 commits into
plotly:masterfrom
n-riesco:editor/use-react-codemirror2

Conversation

@n-riesco

@n-riescon-riesco commented Apr 6, 2018

Copy link
Copy Markdown
Contributor
  • replaced react-codemirror with react-codemirror2
  • used react-resizable to implement a resize handle
  • improved CSS styles (CodeEditor, SQLTable and tab list)
  • TODO: add jest test?

Closes#401
Closes#360
Closes#311
Closes#412

peek 2018-04-06 23-32

* Implemented CodeEditor using `react-codemirror2`.
Closesplotly#401Closesplotly#360
* Uses react-resizable
Closesplotly#311
* Removed component CodeEditorField.
* Removed dependency react-codemirror.
@n-riesco
n-riescoforce-pushed the editor/use-react-codemirror2 branch from 586f28e to dc37d26CompareApril 10, 2018 16:51
* 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
n-riescoforce-pushed the editor/use-react-codemirror2 branch from dc37d26 to c4f215dCompareApril 10, 2018 16:58
@n-riesco

n-riesco commented Apr 10, 2018

Copy link
Copy Markdown
ContributorAuthor

@jackparmer@shannonlal I've updated the PR with a few, small fixes I want to merge before we release Falcon v2.6:

  • I've made the initial size of the code editor smaller and the table view accordingly larger.
  • the table view now uses the same color theme as the code editor.
  • I've changed the way to toggle the row filter (I've a added a link similar to the one used to toggle the code editor).
  • I've updated ibm_dbto v2.3 (no need for my fork any longer!!!)

Please, let me knwo what you think of the UI changes.

peek 2018-04-10 18-29

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be moving connection status to a centralized location or atleast have this as a const? Minor thing

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's not do that. There is no gain in having STATUS_OK_200 vs 200.

maxConstraints
} = this.state;

const mode = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add other dialects should the be added here? Should we update the NEW_CONNECTION doc?


minWidth={740}
minHeight={200}
minHeight={300}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing. Do these variables belong in const?

@n-riescon-riescoApr 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 const if it documents its meaning; e.g. in minHeight={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.

@shannonlal

Copy link
Copy Markdown
Contributor

Added minor comments that you can review if you have time 💃

@n-riesco
n-riesco merged commit 96dfe32 into plotly:masterApr 11, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@n-riesco@shannonlal