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

csv: implement file drag and drop - #385

Merged
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size
Feb 27, 2018
Merged

csv: implement file drag and drop#385
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size

Conversation

@n-riesco

@n-riescon-riesco commented Feb 22, 2018

Copy link
Copy Markdown
Contributor
  • Accept and parse data URLs.

  • Added component <Filedrop>, so that it's possible to drop files into the URL box.

  • Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.

  • Added Datastores.disconnect(connection) to be invoked when a connection's tab is closed.

  • Implemented disconnect for CSV connectors, so that closing a connection's tab removes the CSV file from storage,

  • Converted CSV module to CommonJS, so that all the files that use
    const CSV = require('./csv.js') share the same CSV storage.

  • Remove bad connection files at startup.

Closes#383

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 9c24a81 to 0599eb1CompareFebruary 22, 2018 00:16
* Added function `setSize(bytes)` to limit the size of the CSV storage.
* Converted CSV module to CommonJS, so that all the files that use
`const CSV = require('.csv.js')` share the same CSV storage.
* Added spec to test `setSize(bytes)`.
* Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 115b1db to 37b4110CompareFebruary 22, 2018 16:04
@n-riescon-riesco changed the title csv: limit storage sizecsv: data URLS and limit storage sizeFeb 22, 2018
@n-riescon-riesco changed the title csv: data URLS and limit storage sizecsv: data URLs and limit storage sizeFeb 22, 2018
* `connect` accepts and parses data URLs.
* Added spec to test connection to a data URL.
* Renamed `setSize` to `setStorageSize`.
* Removed CSVError.
* Now it's possible to drag files and drop them on the URL box.
* Removed packages `data-urls` and `whatwg-encoding`, because they would
exhaust memory when parsing data URLs of 20MB.
* Improved the generation of a connection label when using data URLs.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 37b4110 to e7dc726CompareFebruary 23, 2018 09:04
* Added method Datastores.disconnect(connection) that gets invoked
whenever a connection is removed by deleteConnectionById(id).
* Implemented disconnect for CSV connector, so that a disconnection
removes the CSV file from the storage.
* Added spec for disconnect.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from e7dc726 to 30f4161CompareFebruary 23, 2018 09:19
@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal This PR extends the CSV connector. Now, it's possible to drag files into the URL box. To do so, I've added a new component: <Filedrop>.

Another interesting thing in this PR is that I've extended the Datastores interface. Now, connectors can implement the method disconnect. disconnect is invoked when a connection tab is closed.


Would you like to review this PR when you find the time?

@n-riescon-riesco changed the title csv: data URLs and limit storage sizecsv: implement file drag and dropFeb 23, 2018

@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.

@n-riesco Really good job. Most are my comments are just minor documentation/questions. I think it is good to go. 💃

}

/**
* Filedrop is an input component where users can type an URL or drop a file

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.

General Comment. I like the style how you have documented the JSDocs and React class structure. I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

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.

@shannonlal

I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

We don't have any guidelines. I'd keep them in a file: CODING_GUIDELINES.md.

I don't want the guidelines to become a burden (it's easier for us to tell contributors to use given files as models; e.g: we could chose one of the files in the project as a model for stateless components, another file for stateful components, and another for components that use redux). Also, by burden, I mean:

  • For example, the burden of prescribing that PropTypes go at the top is OK, because it's just a copy'n'paste.
  • But, for example, I don't think the burden of prescribing all React components should be class components is worth it (i.e. it's OK, and some would say better, that stateless components are written as functional components).


/**
* Filedrop is an input component where users can type an URL or drop a file
*

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.

Code looks clean and easy to understand. Do you think this component is worth having a Jest test for or is this overkill?

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.

I've just added the tests

/*
* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and

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.

Is this JSDocs format? Should it be @params {object} connection

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.

I've added a commit to format it as JSDoc

* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and
* returns a promise that resolves to the connection object

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.

@returns {promise} an empty promise

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.

This depends on the connector and it's likely to change once Falcon becomes smarter about the connections it creates (at the moment, most of the connectors create a new connection for every request, which is OK if the DB driver implements a pool or the request is stateless).

I'll reword it, so that it only states that the promise resolves once the connection succeeds.

}


/**

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.

It looks like you have two comment blocks for this function

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.

The first block declares the type CSVConnection and the second block declares the function connect. See that CSVConnection is also used in the declarations of disconnect, query, tables and schemas.

@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal I've added tests for <Filedrop>. I'll wait for you to review them before merging.

@@ -0,0 +1,135 @@
jest.unmock('../../../../../app/components/Settings/UserConnections/filedrop.jsx');

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.

Really good job

@shannonlal

Copy link
Copy Markdown
Contributor

@n-riesco Really good job. 💃 . Just curious. Your thoughts about Jest and enzyme as UI testing framework?

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from efae52c to 918d39cCompareFebruary 27, 2018 08:59
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 918d39c to f0effc2CompareFebruary 27, 2018 09:10
@n-riesco

n-riesco commented Feb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

@shannonlal

Just curious. Your thoughts about Jest and enzyme as UI testing framework?

Testing the initial mount is convenient, but:

  • testing subsequent changes can be rather repetitive (this is the reason why I defined getCurrentInput())
  • and testing asynchronous changes can be rather hacky (I couldn't find any API to register a callback to be triggered when a component is updated)

@n-riesco
n-riesco merged commit 8d75c91 into plotly:masterFeb 27, 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.

CSV: let users upload a CSV file

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.

csv: implement file drag and drop - #385

Merged
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size
Feb 27, 2018
Merged

csv: implement file drag and drop#385
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size

Conversation

@n-riesco

@n-riescon-riesco commented Feb 22, 2018

Copy link
Copy Markdown
Contributor
  • Accept and parse data URLs.

  • Added component <Filedrop>, so that it's possible to drop files into the URL box.

  • Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.

  • Added Datastores.disconnect(connection) to be invoked when a connection's tab is closed.

  • Implemented disconnect for CSV connectors, so that closing a connection's tab removes the CSV file from storage,

  • Converted CSV module to CommonJS, so that all the files that use
    const CSV = require('./csv.js') share the same CSV storage.

  • Remove bad connection files at startup.

Closes#383

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 9c24a81 to 0599eb1CompareFebruary 22, 2018 00:16
* Added function `setSize(bytes)` to limit the size of the CSV storage.
* Converted CSV module to CommonJS, so that all the files that use
`const CSV = require('.csv.js')` share the same CSV storage.
* Added spec to test `setSize(bytes)`.
* Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 115b1db to 37b4110CompareFebruary 22, 2018 16:04
@n-riescon-riesco changed the title csv: limit storage sizecsv: data URLS and limit storage sizeFeb 22, 2018
@n-riescon-riesco changed the title csv: data URLS and limit storage sizecsv: data URLs and limit storage sizeFeb 22, 2018
* `connect` accepts and parses data URLs.
* Added spec to test connection to a data URL.
* Renamed `setSize` to `setStorageSize`.
* Removed CSVError.
* Now it's possible to drag files and drop them on the URL box.
* Removed packages `data-urls` and `whatwg-encoding`, because they would
exhaust memory when parsing data URLs of 20MB.
* Improved the generation of a connection label when using data URLs.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 37b4110 to e7dc726CompareFebruary 23, 2018 09:04
* Added method Datastores.disconnect(connection) that gets invoked
whenever a connection is removed by deleteConnectionById(id).
* Implemented disconnect for CSV connector, so that a disconnection
removes the CSV file from the storage.
* Added spec for disconnect.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from e7dc726 to 30f4161CompareFebruary 23, 2018 09:19
@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal This PR extends the CSV connector. Now, it's possible to drag files into the URL box. To do so, I've added a new component: <Filedrop>.

Another interesting thing in this PR is that I've extended the Datastores interface. Now, connectors can implement the method disconnect. disconnect is invoked when a connection tab is closed.


Would you like to review this PR when you find the time?

@n-riescon-riesco changed the title csv: data URLs and limit storage sizecsv: implement file drag and dropFeb 23, 2018

@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.

@n-riesco Really good job. Most are my comments are just minor documentation/questions. I think it is good to go. 💃

}

/**
* Filedrop is an input component where users can type an URL or drop a file

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.

General Comment. I like the style how you have documented the JSDocs and React class structure. I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

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.

@shannonlal

I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

We don't have any guidelines. I'd keep them in a file: CODING_GUIDELINES.md.

I don't want the guidelines to become a burden (it's easier for us to tell contributors to use given files as models; e.g: we could chose one of the files in the project as a model for stateless components, another file for stateful components, and another for components that use redux). Also, by burden, I mean:

  • For example, the burden of prescribing that PropTypes go at the top is OK, because it's just a copy'n'paste.
  • But, for example, I don't think the burden of prescribing all React components should be class components is worth it (i.e. it's OK, and some would say better, that stateless components are written as functional components).


/**
* Filedrop is an input component where users can type an URL or drop a file
*

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.

Code looks clean and easy to understand. Do you think this component is worth having a Jest test for or is this overkill?

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.

I've just added the tests

/*
* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and

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.

Is this JSDocs format? Should it be @params {object} connection

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.

I've added a commit to format it as JSDoc

* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and
* returns a promise that resolves to the connection object

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.

@returns {promise} an empty promise

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.

This depends on the connector and it's likely to change once Falcon becomes smarter about the connections it creates (at the moment, most of the connectors create a new connection for every request, which is OK if the DB driver implements a pool or the request is stateless).

I'll reword it, so that it only states that the promise resolves once the connection succeeds.

}


/**

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.

It looks like you have two comment blocks for this function

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.

The first block declares the type CSVConnection and the second block declares the function connect. See that CSVConnection is also used in the declarations of disconnect, query, tables and schemas.

@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal I've added tests for <Filedrop>. I'll wait for you to review them before merging.

@@ -0,0 +1,135 @@
jest.unmock('../../../../../app/components/Settings/UserConnections/filedrop.jsx');

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.

Really good job

@shannonlal

Copy link
Copy Markdown
Contributor

@n-riesco Really good job. 💃 . Just curious. Your thoughts about Jest and enzyme as UI testing framework?

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from efae52c to 918d39cCompareFebruary 27, 2018 08:59
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 918d39c to f0effc2CompareFebruary 27, 2018 09:10
@n-riesco

n-riesco commented Feb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

@shannonlal

Just curious. Your thoughts about Jest and enzyme as UI testing framework?

Testing the initial mount is convenient, but:

  • testing subsequent changes can be rather repetitive (this is the reason why I defined getCurrentInput())
  • and testing asynchronous changes can be rather hacky (I couldn't find any API to register a callback to be triggered when a component is updated)

@n-riesco
n-riesco merged commit 8d75c91 into plotly:masterFeb 27, 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.

CSV: let users upload a CSV file

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.

csv: implement file drag and drop - #385

Merged
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size
Feb 27, 2018
Merged

csv: implement file drag and drop#385
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size

Conversation

@n-riesco

@n-riescon-riesco commented Feb 22, 2018

Copy link
Copy Markdown
Contributor
  • Accept and parse data URLs.

  • Added component <Filedrop>, so that it's possible to drop files into the URL box.

  • Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.

  • Added Datastores.disconnect(connection) to be invoked when a connection's tab is closed.

  • Implemented disconnect for CSV connectors, so that closing a connection's tab removes the CSV file from storage,

  • Converted CSV module to CommonJS, so that all the files that use
    const CSV = require('./csv.js') share the same CSV storage.

  • Remove bad connection files at startup.

Closes#383

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 9c24a81 to 0599eb1CompareFebruary 22, 2018 00:16
* Added function `setSize(bytes)` to limit the size of the CSV storage.
* Converted CSV module to CommonJS, so that all the files that use
`const CSV = require('.csv.js')` share the same CSV storage.
* Added spec to test `setSize(bytes)`.
* Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 115b1db to 37b4110CompareFebruary 22, 2018 16:04
@n-riescon-riesco changed the title csv: limit storage sizecsv: data URLS and limit storage sizeFeb 22, 2018
@n-riescon-riesco changed the title csv: data URLS and limit storage sizecsv: data URLs and limit storage sizeFeb 22, 2018
* `connect` accepts and parses data URLs.
* Added spec to test connection to a data URL.
* Renamed `setSize` to `setStorageSize`.
* Removed CSVError.
* Now it's possible to drag files and drop them on the URL box.
* Removed packages `data-urls` and `whatwg-encoding`, because they would
exhaust memory when parsing data URLs of 20MB.
* Improved the generation of a connection label when using data URLs.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 37b4110 to e7dc726CompareFebruary 23, 2018 09:04
* Added method Datastores.disconnect(connection) that gets invoked
whenever a connection is removed by deleteConnectionById(id).
* Implemented disconnect for CSV connector, so that a disconnection
removes the CSV file from the storage.
* Added spec for disconnect.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from e7dc726 to 30f4161CompareFebruary 23, 2018 09:19
@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal This PR extends the CSV connector. Now, it's possible to drag files into the URL box. To do so, I've added a new component: <Filedrop>.

Another interesting thing in this PR is that I've extended the Datastores interface. Now, connectors can implement the method disconnect. disconnect is invoked when a connection tab is closed.


Would you like to review this PR when you find the time?

@n-riescon-riesco changed the title csv: data URLs and limit storage sizecsv: implement file drag and dropFeb 23, 2018

@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.

@n-riesco Really good job. Most are my comments are just minor documentation/questions. I think it is good to go. 💃

}

/**
* Filedrop is an input component where users can type an URL or drop a file

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.

General Comment. I like the style how you have documented the JSDocs and React class structure. I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

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.

@shannonlal

I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

We don't have any guidelines. I'd keep them in a file: CODING_GUIDELINES.md.

I don't want the guidelines to become a burden (it's easier for us to tell contributors to use given files as models; e.g: we could chose one of the files in the project as a model for stateless components, another file for stateful components, and another for components that use redux). Also, by burden, I mean:

  • For example, the burden of prescribing that PropTypes go at the top is OK, because it's just a copy'n'paste.
  • But, for example, I don't think the burden of prescribing all React components should be class components is worth it (i.e. it's OK, and some would say better, that stateless components are written as functional components).


/**
* Filedrop is an input component where users can type an URL or drop a file
*

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.

Code looks clean and easy to understand. Do you think this component is worth having a Jest test for or is this overkill?

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.

I've just added the tests

/*
* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and

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.

Is this JSDocs format? Should it be @params {object} connection

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.

I've added a commit to format it as JSDoc

* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and
* returns a promise that resolves to the connection object

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.

@returns {promise} an empty promise

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.

This depends on the connector and it's likely to change once Falcon becomes smarter about the connections it creates (at the moment, most of the connectors create a new connection for every request, which is OK if the DB driver implements a pool or the request is stateless).

I'll reword it, so that it only states that the promise resolves once the connection succeeds.

}


/**

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.

It looks like you have two comment blocks for this function

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.

The first block declares the type CSVConnection and the second block declares the function connect. See that CSVConnection is also used in the declarations of disconnect, query, tables and schemas.

@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal I've added tests for <Filedrop>. I'll wait for you to review them before merging.

@@ -0,0 +1,135 @@
jest.unmock('../../../../../app/components/Settings/UserConnections/filedrop.jsx');

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.

Really good job

@shannonlal

Copy link
Copy Markdown
Contributor

@n-riesco Really good job. 💃 . Just curious. Your thoughts about Jest and enzyme as UI testing framework?

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from efae52c to 918d39cCompareFebruary 27, 2018 08:59
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 918d39c to f0effc2CompareFebruary 27, 2018 09:10
@n-riesco

n-riesco commented Feb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

@shannonlal

Just curious. Your thoughts about Jest and enzyme as UI testing framework?

Testing the initial mount is convenient, but:

  • testing subsequent changes can be rather repetitive (this is the reason why I defined getCurrentInput())
  • and testing asynchronous changes can be rather hacky (I couldn't find any API to register a callback to be triggered when a component is updated)

@n-riesco
n-riesco merged commit 8d75c91 into plotly:masterFeb 27, 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.

CSV: let users upload a CSV file

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.

csv: implement file drag and drop - #385

Merged
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size
Feb 27, 2018
Merged

csv: implement file drag and drop#385
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size

Conversation

@n-riesco

@n-riescon-riesco commented Feb 22, 2018

Copy link
Copy Markdown
Contributor
  • Accept and parse data URLs.

  • Added component <Filedrop>, so that it's possible to drop files into the URL box.

  • Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.

  • Added Datastores.disconnect(connection) to be invoked when a connection's tab is closed.

  • Implemented disconnect for CSV connectors, so that closing a connection's tab removes the CSV file from storage,

  • Converted CSV module to CommonJS, so that all the files that use
    const CSV = require('./csv.js') share the same CSV storage.

  • Remove bad connection files at startup.

Closes#383

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 9c24a81 to 0599eb1CompareFebruary 22, 2018 00:16
* Added function `setSize(bytes)` to limit the size of the CSV storage.
* Converted CSV module to CommonJS, so that all the files that use
`const CSV = require('.csv.js')` share the same CSV storage.
* Added spec to test `setSize(bytes)`.
* Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 115b1db to 37b4110CompareFebruary 22, 2018 16:04
@n-riescon-riesco changed the title csv: limit storage sizecsv: data URLS and limit storage sizeFeb 22, 2018
@n-riescon-riesco changed the title csv: data URLS and limit storage sizecsv: data URLs and limit storage sizeFeb 22, 2018
* `connect` accepts and parses data URLs.
* Added spec to test connection to a data URL.
* Renamed `setSize` to `setStorageSize`.
* Removed CSVError.
* Now it's possible to drag files and drop them on the URL box.
* Removed packages `data-urls` and `whatwg-encoding`, because they would
exhaust memory when parsing data URLs of 20MB.
* Improved the generation of a connection label when using data URLs.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 37b4110 to e7dc726CompareFebruary 23, 2018 09:04
* Added method Datastores.disconnect(connection) that gets invoked
whenever a connection is removed by deleteConnectionById(id).
* Implemented disconnect for CSV connector, so that a disconnection
removes the CSV file from the storage.
* Added spec for disconnect.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from e7dc726 to 30f4161CompareFebruary 23, 2018 09:19
@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal This PR extends the CSV connector. Now, it's possible to drag files into the URL box. To do so, I've added a new component: <Filedrop>.

Another interesting thing in this PR is that I've extended the Datastores interface. Now, connectors can implement the method disconnect. disconnect is invoked when a connection tab is closed.


Would you like to review this PR when you find the time?

@n-riescon-riesco changed the title csv: data URLs and limit storage sizecsv: implement file drag and dropFeb 23, 2018

@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.

@n-riesco Really good job. Most are my comments are just minor documentation/questions. I think it is good to go. 💃

}

/**
* Filedrop is an input component where users can type an URL or drop a file

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.

General Comment. I like the style how you have documented the JSDocs and React class structure. I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

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.

@shannonlal

I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

We don't have any guidelines. I'd keep them in a file: CODING_GUIDELINES.md.

I don't want the guidelines to become a burden (it's easier for us to tell contributors to use given files as models; e.g: we could chose one of the files in the project as a model for stateless components, another file for stateful components, and another for components that use redux). Also, by burden, I mean:

  • For example, the burden of prescribing that PropTypes go at the top is OK, because it's just a copy'n'paste.
  • But, for example, I don't think the burden of prescribing all React components should be class components is worth it (i.e. it's OK, and some would say better, that stateless components are written as functional components).


/**
* Filedrop is an input component where users can type an URL or drop a file
*

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.

Code looks clean and easy to understand. Do you think this component is worth having a Jest test for or is this overkill?

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.

I've just added the tests

/*
* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and

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.

Is this JSDocs format? Should it be @params {object} connection

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.

I've added a commit to format it as JSDoc

* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and
* returns a promise that resolves to the connection object

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.

@returns {promise} an empty promise

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.

This depends on the connector and it's likely to change once Falcon becomes smarter about the connections it creates (at the moment, most of the connectors create a new connection for every request, which is OK if the DB driver implements a pool or the request is stateless).

I'll reword it, so that it only states that the promise resolves once the connection succeeds.

}


/**

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.

It looks like you have two comment blocks for this function

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.

The first block declares the type CSVConnection and the second block declares the function connect. See that CSVConnection is also used in the declarations of disconnect, query, tables and schemas.

@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal I've added tests for <Filedrop>. I'll wait for you to review them before merging.

@@ -0,0 +1,135 @@
jest.unmock('../../../../../app/components/Settings/UserConnections/filedrop.jsx');

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.

Really good job

@shannonlal

Copy link
Copy Markdown
Contributor

@n-riesco Really good job. 💃 . Just curious. Your thoughts about Jest and enzyme as UI testing framework?

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from efae52c to 918d39cCompareFebruary 27, 2018 08:59
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 918d39c to f0effc2CompareFebruary 27, 2018 09:10
@n-riesco

n-riesco commented Feb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

@shannonlal

Just curious. Your thoughts about Jest and enzyme as UI testing framework?

Testing the initial mount is convenient, but:

  • testing subsequent changes can be rather repetitive (this is the reason why I defined getCurrentInput())
  • and testing asynchronous changes can be rather hacky (I couldn't find any API to register a callback to be triggered when a component is updated)

@n-riesco
n-riesco merged commit 8d75c91 into plotly:masterFeb 27, 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.

CSV: let users upload a CSV file

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.

csv: implement file drag and drop - #385

Merged
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size
Feb 27, 2018
Merged

csv: implement file drag and drop#385
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size

Conversation

@n-riesco

@n-riescon-riesco commented Feb 22, 2018

Copy link
Copy Markdown
Contributor
  • Accept and parse data URLs.

  • Added component <Filedrop>, so that it's possible to drop files into the URL box.

  • Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.

  • Added Datastores.disconnect(connection) to be invoked when a connection's tab is closed.

  • Implemented disconnect for CSV connectors, so that closing a connection's tab removes the CSV file from storage,

  • Converted CSV module to CommonJS, so that all the files that use
    const CSV = require('./csv.js') share the same CSV storage.

  • Remove bad connection files at startup.

Closes#383

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 9c24a81 to 0599eb1CompareFebruary 22, 2018 00:16
* Added function `setSize(bytes)` to limit the size of the CSV storage.
* Converted CSV module to CommonJS, so that all the files that use
`const CSV = require('.csv.js')` share the same CSV storage.
* Added spec to test `setSize(bytes)`.
* Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 115b1db to 37b4110CompareFebruary 22, 2018 16:04
@n-riescon-riesco changed the title csv: limit storage sizecsv: data URLS and limit storage sizeFeb 22, 2018
@n-riescon-riesco changed the title csv: data URLS and limit storage sizecsv: data URLs and limit storage sizeFeb 22, 2018
* `connect` accepts and parses data URLs.
* Added spec to test connection to a data URL.
* Renamed `setSize` to `setStorageSize`.
* Removed CSVError.
* Now it's possible to drag files and drop them on the URL box.
* Removed packages `data-urls` and `whatwg-encoding`, because they would
exhaust memory when parsing data URLs of 20MB.
* Improved the generation of a connection label when using data URLs.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 37b4110 to e7dc726CompareFebruary 23, 2018 09:04
* Added method Datastores.disconnect(connection) that gets invoked
whenever a connection is removed by deleteConnectionById(id).
* Implemented disconnect for CSV connector, so that a disconnection
removes the CSV file from the storage.
* Added spec for disconnect.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from e7dc726 to 30f4161CompareFebruary 23, 2018 09:19
@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal This PR extends the CSV connector. Now, it's possible to drag files into the URL box. To do so, I've added a new component: <Filedrop>.

Another interesting thing in this PR is that I've extended the Datastores interface. Now, connectors can implement the method disconnect. disconnect is invoked when a connection tab is closed.


Would you like to review this PR when you find the time?

@n-riescon-riesco changed the title csv: data URLs and limit storage sizecsv: implement file drag and dropFeb 23, 2018

@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.

@n-riesco Really good job. Most are my comments are just minor documentation/questions. I think it is good to go. 💃

}

/**
* Filedrop is an input component where users can type an URL or drop a file

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.

General Comment. I like the style how you have documented the JSDocs and React class structure. I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

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.

@shannonlal

I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

We don't have any guidelines. I'd keep them in a file: CODING_GUIDELINES.md.

I don't want the guidelines to become a burden (it's easier for us to tell contributors to use given files as models; e.g: we could chose one of the files in the project as a model for stateless components, another file for stateful components, and another for components that use redux). Also, by burden, I mean:

  • For example, the burden of prescribing that PropTypes go at the top is OK, because it's just a copy'n'paste.
  • But, for example, I don't think the burden of prescribing all React components should be class components is worth it (i.e. it's OK, and some would say better, that stateless components are written as functional components).


/**
* Filedrop is an input component where users can type an URL or drop a file
*

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.

Code looks clean and easy to understand. Do you think this component is worth having a Jest test for or is this overkill?

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.

I've just added the tests

/*
* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and

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.

Is this JSDocs format? Should it be @params {object} connection

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.

I've added a commit to format it as JSDoc

* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and
* returns a promise that resolves to the connection object

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.

@returns {promise} an empty promise

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.

This depends on the connector and it's likely to change once Falcon becomes smarter about the connections it creates (at the moment, most of the connectors create a new connection for every request, which is OK if the DB driver implements a pool or the request is stateless).

I'll reword it, so that it only states that the promise resolves once the connection succeeds.

}


/**

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.

It looks like you have two comment blocks for this function

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.

The first block declares the type CSVConnection and the second block declares the function connect. See that CSVConnection is also used in the declarations of disconnect, query, tables and schemas.

@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal I've added tests for <Filedrop>. I'll wait for you to review them before merging.

@@ -0,0 +1,135 @@
jest.unmock('../../../../../app/components/Settings/UserConnections/filedrop.jsx');

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.

Really good job

@shannonlal

Copy link
Copy Markdown
Contributor

@n-riesco Really good job. 💃 . Just curious. Your thoughts about Jest and enzyme as UI testing framework?

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from efae52c to 918d39cCompareFebruary 27, 2018 08:59
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 918d39c to f0effc2CompareFebruary 27, 2018 09:10
@n-riesco

n-riesco commented Feb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

@shannonlal

Just curious. Your thoughts about Jest and enzyme as UI testing framework?

Testing the initial mount is convenient, but:

  • testing subsequent changes can be rather repetitive (this is the reason why I defined getCurrentInput())
  • and testing asynchronous changes can be rather hacky (I couldn't find any API to register a callback to be triggered when a component is updated)

@n-riesco
n-riesco merged commit 8d75c91 into plotly:masterFeb 27, 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.

CSV: let users upload a CSV file

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.

csv: implement file drag and drop - #385

Merged
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size
Feb 27, 2018
Merged

csv: implement file drag and drop#385
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size

Conversation

@n-riesco

@n-riescon-riesco commented Feb 22, 2018

Copy link
Copy Markdown
Contributor
  • Accept and parse data URLs.

  • Added component <Filedrop>, so that it's possible to drop files into the URL box.

  • Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.

  • Added Datastores.disconnect(connection) to be invoked when a connection's tab is closed.

  • Implemented disconnect for CSV connectors, so that closing a connection's tab removes the CSV file from storage,

  • Converted CSV module to CommonJS, so that all the files that use
    const CSV = require('./csv.js') share the same CSV storage.

  • Remove bad connection files at startup.

Closes#383

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 9c24a81 to 0599eb1CompareFebruary 22, 2018 00:16
* Added function `setSize(bytes)` to limit the size of the CSV storage.
* Converted CSV module to CommonJS, so that all the files that use
`const CSV = require('.csv.js')` share the same CSV storage.
* Added spec to test `setSize(bytes)`.
* Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 115b1db to 37b4110CompareFebruary 22, 2018 16:04
@n-riescon-riesco changed the title csv: limit storage sizecsv: data URLS and limit storage sizeFeb 22, 2018
@n-riescon-riesco changed the title csv: data URLS and limit storage sizecsv: data URLs and limit storage sizeFeb 22, 2018
* `connect` accepts and parses data URLs.
* Added spec to test connection to a data URL.
* Renamed `setSize` to `setStorageSize`.
* Removed CSVError.
* Now it's possible to drag files and drop them on the URL box.
* Removed packages `data-urls` and `whatwg-encoding`, because they would
exhaust memory when parsing data URLs of 20MB.
* Improved the generation of a connection label when using data URLs.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 37b4110 to e7dc726CompareFebruary 23, 2018 09:04
* Added method Datastores.disconnect(connection) that gets invoked
whenever a connection is removed by deleteConnectionById(id).
* Implemented disconnect for CSV connector, so that a disconnection
removes the CSV file from the storage.
* Added spec for disconnect.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from e7dc726 to 30f4161CompareFebruary 23, 2018 09:19
@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal This PR extends the CSV connector. Now, it's possible to drag files into the URL box. To do so, I've added a new component: <Filedrop>.

Another interesting thing in this PR is that I've extended the Datastores interface. Now, connectors can implement the method disconnect. disconnect is invoked when a connection tab is closed.


Would you like to review this PR when you find the time?

@n-riescon-riesco changed the title csv: data URLs and limit storage sizecsv: implement file drag and dropFeb 23, 2018

@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.

@n-riesco Really good job. Most are my comments are just minor documentation/questions. I think it is good to go. 💃

}

/**
* Filedrop is an input component where users can type an URL or drop a file

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.

General Comment. I like the style how you have documented the JSDocs and React class structure. I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

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.

@shannonlal

I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

We don't have any guidelines. I'd keep them in a file: CODING_GUIDELINES.md.

I don't want the guidelines to become a burden (it's easier for us to tell contributors to use given files as models; e.g: we could chose one of the files in the project as a model for stateless components, another file for stateful components, and another for components that use redux). Also, by burden, I mean:

  • For example, the burden of prescribing that PropTypes go at the top is OK, because it's just a copy'n'paste.
  • But, for example, I don't think the burden of prescribing all React components should be class components is worth it (i.e. it's OK, and some would say better, that stateless components are written as functional components).


/**
* Filedrop is an input component where users can type an URL or drop a file
*

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.

Code looks clean and easy to understand. Do you think this component is worth having a Jest test for or is this overkill?

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.

I've just added the tests

/*
* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and

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.

Is this JSDocs format? Should it be @params {object} connection

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.

I've added a commit to format it as JSDoc

* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and
* returns a promise that resolves to the connection object

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.

@returns {promise} an empty promise

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.

This depends on the connector and it's likely to change once Falcon becomes smarter about the connections it creates (at the moment, most of the connectors create a new connection for every request, which is OK if the DB driver implements a pool or the request is stateless).

I'll reword it, so that it only states that the promise resolves once the connection succeeds.

}


/**

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.

It looks like you have two comment blocks for this function

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.

The first block declares the type CSVConnection and the second block declares the function connect. See that CSVConnection is also used in the declarations of disconnect, query, tables and schemas.

@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal I've added tests for <Filedrop>. I'll wait for you to review them before merging.

@@ -0,0 +1,135 @@
jest.unmock('../../../../../app/components/Settings/UserConnections/filedrop.jsx');

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.

Really good job

@shannonlal

Copy link
Copy Markdown
Contributor

@n-riesco Really good job. 💃 . Just curious. Your thoughts about Jest and enzyme as UI testing framework?

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from efae52c to 918d39cCompareFebruary 27, 2018 08:59
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 918d39c to f0effc2CompareFebruary 27, 2018 09:10
@n-riesco

n-riesco commented Feb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

@shannonlal

Just curious. Your thoughts about Jest and enzyme as UI testing framework?

Testing the initial mount is convenient, but:

  • testing subsequent changes can be rather repetitive (this is the reason why I defined getCurrentInput())
  • and testing asynchronous changes can be rather hacky (I couldn't find any API to register a callback to be triggered when a component is updated)

@n-riesco
n-riesco merged commit 8d75c91 into plotly:masterFeb 27, 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.

CSV: let users upload a CSV file

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.

csv: implement file drag and drop - #385

Merged
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size
Feb 27, 2018
Merged

csv: implement file drag and drop#385
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size

Conversation

@n-riesco

@n-riescon-riesco commented Feb 22, 2018

Copy link
Copy Markdown
Contributor
  • Accept and parse data URLs.

  • Added component <Filedrop>, so that it's possible to drop files into the URL box.

  • Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.

  • Added Datastores.disconnect(connection) to be invoked when a connection's tab is closed.

  • Implemented disconnect for CSV connectors, so that closing a connection's tab removes the CSV file from storage,

  • Converted CSV module to CommonJS, so that all the files that use
    const CSV = require('./csv.js') share the same CSV storage.

  • Remove bad connection files at startup.

Closes#383

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 9c24a81 to 0599eb1CompareFebruary 22, 2018 00:16
* Added function `setSize(bytes)` to limit the size of the CSV storage.
* Converted CSV module to CommonJS, so that all the files that use
`const CSV = require('.csv.js')` share the same CSV storage.
* Added spec to test `setSize(bytes)`.
* Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 115b1db to 37b4110CompareFebruary 22, 2018 16:04
@n-riescon-riesco changed the title csv: limit storage sizecsv: data URLS and limit storage sizeFeb 22, 2018
@n-riescon-riesco changed the title csv: data URLS and limit storage sizecsv: data URLs and limit storage sizeFeb 22, 2018
* `connect` accepts and parses data URLs.
* Added spec to test connection to a data URL.
* Renamed `setSize` to `setStorageSize`.
* Removed CSVError.
* Now it's possible to drag files and drop them on the URL box.
* Removed packages `data-urls` and `whatwg-encoding`, because they would
exhaust memory when parsing data URLs of 20MB.
* Improved the generation of a connection label when using data URLs.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 37b4110 to e7dc726CompareFebruary 23, 2018 09:04
* Added method Datastores.disconnect(connection) that gets invoked
whenever a connection is removed by deleteConnectionById(id).
* Implemented disconnect for CSV connector, so that a disconnection
removes the CSV file from the storage.
* Added spec for disconnect.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from e7dc726 to 30f4161CompareFebruary 23, 2018 09:19
@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal This PR extends the CSV connector. Now, it's possible to drag files into the URL box. To do so, I've added a new component: <Filedrop>.

Another interesting thing in this PR is that I've extended the Datastores interface. Now, connectors can implement the method disconnect. disconnect is invoked when a connection tab is closed.


Would you like to review this PR when you find the time?

@n-riescon-riesco changed the title csv: data URLs and limit storage sizecsv: implement file drag and dropFeb 23, 2018

@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.

@n-riesco Really good job. Most are my comments are just minor documentation/questions. I think it is good to go. 💃

}

/**
* Filedrop is an input component where users can type an URL or drop a file

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.

General Comment. I like the style how you have documented the JSDocs and React class structure. I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

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.

@shannonlal

I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

We don't have any guidelines. I'd keep them in a file: CODING_GUIDELINES.md.

I don't want the guidelines to become a burden (it's easier for us to tell contributors to use given files as models; e.g: we could chose one of the files in the project as a model for stateless components, another file for stateful components, and another for components that use redux). Also, by burden, I mean:

  • For example, the burden of prescribing that PropTypes go at the top is OK, because it's just a copy'n'paste.
  • But, for example, I don't think the burden of prescribing all React components should be class components is worth it (i.e. it's OK, and some would say better, that stateless components are written as functional components).


/**
* Filedrop is an input component where users can type an URL or drop a file
*

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.

Code looks clean and easy to understand. Do you think this component is worth having a Jest test for or is this overkill?

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.

I've just added the tests

/*
* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and

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.

Is this JSDocs format? Should it be @params {object} connection

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.

I've added a commit to format it as JSDoc

* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and
* returns a promise that resolves to the connection object

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.

@returns {promise} an empty promise

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.

This depends on the connector and it's likely to change once Falcon becomes smarter about the connections it creates (at the moment, most of the connectors create a new connection for every request, which is OK if the DB driver implements a pool or the request is stateless).

I'll reword it, so that it only states that the promise resolves once the connection succeeds.

}


/**

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.

It looks like you have two comment blocks for this function

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.

The first block declares the type CSVConnection and the second block declares the function connect. See that CSVConnection is also used in the declarations of disconnect, query, tables and schemas.

@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal I've added tests for <Filedrop>. I'll wait for you to review them before merging.

@@ -0,0 +1,135 @@
jest.unmock('../../../../../app/components/Settings/UserConnections/filedrop.jsx');

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.

Really good job

@shannonlal

Copy link
Copy Markdown
Contributor

@n-riesco Really good job. 💃 . Just curious. Your thoughts about Jest and enzyme as UI testing framework?

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from efae52c to 918d39cCompareFebruary 27, 2018 08:59
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 918d39c to f0effc2CompareFebruary 27, 2018 09:10
@n-riesco

n-riesco commented Feb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

@shannonlal

Just curious. Your thoughts about Jest and enzyme as UI testing framework?

Testing the initial mount is convenient, but:

  • testing subsequent changes can be rather repetitive (this is the reason why I defined getCurrentInput())
  • and testing asynchronous changes can be rather hacky (I couldn't find any API to register a callback to be triggered when a component is updated)

@n-riesco
n-riesco merged commit 8d75c91 into plotly:masterFeb 27, 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.

CSV: let users upload a CSV file

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.

csv: implement file drag and drop - #385

Merged
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size
Feb 27, 2018
Merged

csv: implement file drag and drop#385
n-riesco merged 11 commits into
plotly:masterfrom
n-riesco:csv/limit-storage-size

Conversation

@n-riesco

@n-riescon-riesco commented Feb 22, 2018

Copy link
Copy Markdown
Contributor
  • Accept and parse data URLs.

  • Added component <Filedrop>, so that it's possible to drop files into the URL box.

  • Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.

  • Added Datastores.disconnect(connection) to be invoked when a connection's tab is closed.

  • Implemented disconnect for CSV connectors, so that closing a connection's tab removes the CSV file from storage,

  • Converted CSV module to CommonJS, so that all the files that use
    const CSV = require('./csv.js') share the same CSV storage.

  • Remove bad connection files at startup.

Closes#383

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 9c24a81 to 0599eb1CompareFebruary 22, 2018 00:16
* Added function `setSize(bytes)` to limit the size of the CSV storage.
* Converted CSV module to CommonJS, so that all the files that use
`const CSV = require('.csv.js')` share the same CSV storage.
* Added spec to test `setSize(bytes)`.
* Added setting CSV_STORAGE_SIZE to limit the size of the CSV storage.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 115b1db to 37b4110CompareFebruary 22, 2018 16:04
@n-riescon-riesco changed the title csv: limit storage sizecsv: data URLS and limit storage sizeFeb 22, 2018
@n-riescon-riesco changed the title csv: data URLS and limit storage sizecsv: data URLs and limit storage sizeFeb 22, 2018
* `connect` accepts and parses data URLs.
* Added spec to test connection to a data URL.
* Renamed `setSize` to `setStorageSize`.
* Removed CSVError.
* Now it's possible to drag files and drop them on the URL box.
* Removed packages `data-urls` and `whatwg-encoding`, because they would
exhaust memory when parsing data URLs of 20MB.
* Improved the generation of a connection label when using data URLs.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 37b4110 to e7dc726CompareFebruary 23, 2018 09:04
* Added method Datastores.disconnect(connection) that gets invoked
whenever a connection is removed by deleteConnectionById(id).
* Implemented disconnect for CSV connector, so that a disconnection
removes the CSV file from the storage.
* Added spec for disconnect.
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from e7dc726 to 30f4161CompareFebruary 23, 2018 09:19
@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal This PR extends the CSV connector. Now, it's possible to drag files into the URL box. To do so, I've added a new component: <Filedrop>.

Another interesting thing in this PR is that I've extended the Datastores interface. Now, connectors can implement the method disconnect. disconnect is invoked when a connection tab is closed.


Would you like to review this PR when you find the time?

@n-riescon-riesco changed the title csv: data URLs and limit storage sizecsv: implement file drag and dropFeb 23, 2018

@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.

@n-riesco Really good job. Most are my comments are just minor documentation/questions. I think it is good to go. 💃

}

/**
* Filedrop is an input component where users can type an URL or drop a file

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.

General Comment. I like the style how you have documented the JSDocs and React class structure. I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

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.

@shannonlal

I have noticed that there seems to be some inconsistency with the React Classes. Do we have a standard format? I have noticed some classes have PropTypes at the bottom. Is there a standard? Should we document this somewhere? If so where is a good place?

We don't have any guidelines. I'd keep them in a file: CODING_GUIDELINES.md.

I don't want the guidelines to become a burden (it's easier for us to tell contributors to use given files as models; e.g: we could chose one of the files in the project as a model for stateless components, another file for stateful components, and another for components that use redux). Also, by burden, I mean:

  • For example, the burden of prescribing that PropTypes go at the top is OK, because it's just a copy'n'paste.
  • But, for example, I don't think the burden of prescribing all React components should be class components is worth it (i.e. it's OK, and some would say better, that stateless components are written as functional components).


/**
* Filedrop is an input component where users can type an URL or drop a file
*

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.

Code looks clean and easy to understand. Do you think this component is worth having a Jest test for or is this overkill?

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.

I've just added the tests

/*
* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and

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.

Is this JSDocs format? Should it be @params {object} connection

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.

I've added a commit to format it as JSDoc

* connect functions attempt to ping the connection and
* return a promise that is empty
* connect attempts to ping the connection and
* returns a promise that resolves to the connection object

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.

@returns {promise} an empty promise

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.

This depends on the connector and it's likely to change once Falcon becomes smarter about the connections it creates (at the moment, most of the connectors create a new connection for every request, which is OK if the DB driver implements a pool or the request is stateless).

I'll reword it, so that it only states that the promise resolves once the connection succeeds.

}


/**

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.

It looks like you have two comment blocks for this function

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.

The first block declares the type CSVConnection and the second block declares the function connect. See that CSVConnection is also used in the declarations of disconnect, query, tables and schemas.

@n-riesco

Copy link
Copy Markdown
ContributorAuthor

@shannonlal I've added tests for <Filedrop>. I'll wait for you to review them before merging.

@@ -0,0 +1,135 @@
jest.unmock('../../../../../app/components/Settings/UserConnections/filedrop.jsx');

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.

Really good job

@shannonlal

Copy link
Copy Markdown
Contributor

@n-riesco Really good job. 💃 . Just curious. Your thoughts about Jest and enzyme as UI testing framework?

@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from efae52c to 918d39cCompareFebruary 27, 2018 08:59
@n-riesco
n-riescoforce-pushed the csv/limit-storage-size branch from 918d39c to f0effc2CompareFebruary 27, 2018 09:10
@n-riesco

n-riesco commented Feb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

@shannonlal

Just curious. Your thoughts about Jest and enzyme as UI testing framework?

Testing the initial mount is convenient, but:

  • testing subsequent changes can be rather repetitive (this is the reason why I defined getCurrentInput())
  • and testing asynchronous changes can be rather hacky (I couldn't find any API to register a callback to be triggered when a component is updated)

@n-riesco
n-riesco merged commit 8d75c91 into plotly:masterFeb 27, 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.

CSV: let users upload a CSV file

2 participants

@n-riesco@shannonlal