Skip to content

Conversation

anuveyatsu
Copy link
Member

  • update props of the App container
  • update logic behind App container
  • update fixtures to do dev with cosmos
  • update unit tests

* update props of the App container
* update logic behind App container
* update fixtures to do dev with cosmos
* update unit tests
Copy link
Contributor

@starsinmypockets starsinmypockets left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me.

See my note about exporting the App component from the index file. I'm open to discuss.

src/index.js Outdated
if (!response.ok) {
file.descriptor.unavailable = true
return
const datapackage = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to do this here. We can check this using cosmos or unit tests.

src/index.js Outdated
.catch((error) => {
console.warn('Failed to load a Dataset from provided datapackage id\n' + error)
})
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. I think all of this can go.

src/index.js Outdated
)
})
}
ReactDOM.render(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this either. I think we just need:

export default App

so that we can load this in the parent app (Data Explorer, for instance) like this:

import FilterUI from './components/FilterUI'
import DataView from 'datapackage-views'
import DataViewBuilder from './components/DataViewBuilder'
import { filterUIAction, fetchDataAction, dataViewBuilderAction } from './actions/';

export const App = props => {
  
  return (
    <div className="text-center ml-6">
      <header>
        <div className="container">
          <h1 className="text-3xl">Data Explorer</h1>
        </div>
      </header>
      <div className="container py-6 bg-pink-100">
        <div className="">
          <FilterUI {...props} />
        </div>
      </div>
      <div className="container flex py-6 bg-yellow-100">
        <div className="w-3/4 p-4 mr-4 overflow-x-auto">
          <DataView {...props} />
        </div>
        <div className="w-1/4 p-4 mr-4 bg-green-100">
          <DataViewBuilder {...props} />
        </div>
      </div>
     </div>
  )
}

We might then need to add a wrapper that includes this App for Datahub.io or Preview pages, but I think the module should just export the component.

starsinmypockets and others added 3 commits August 13, 2019 19:14
* Add babel resources to package.json
* Add `yarn build:package` command
* Add updated resources at /dist
@anuveyatsu
Copy link
Member Author

@starsinmypockets before merging this we need to make sure that other projects won't break, e.g., this library is now being used for several projects.

@anuveyatsu anuveyatsu self-assigned this Aug 20, 2019
…ests.

This means we aren't testing Chart compnent.

Error details:

  ● Test suite failed to run

    TypeError: Cannot read property 'document' of undefined

      1 | import React from 'react';
    > 2 | import Plotly from 'plotly.js-basic-dist';
        | ^
      3 | import createPlotlyComponent from "react-plotly.js/factory";
      4 |
      5 |

      at document (node_modules/plotly.js-basic-dist/plotly-basic.js:667:26)
      at Object.7 (node_modules/plotly.js-basic-dist/plotly-basic.js:660:2)
      at call (node_modules/plotly.js-basic-dist/plotly-basic.js:7:631)
      at o (node_modules/plotly.js-basic-dist/plotly-basic.js:7:682)
      at Object._dereq_ (node_modules/plotly.js-basic-dist/plotly-basic.js:36595:10)
      at call (node_modules/plotly.js-basic-dist/plotly-basic.js:7:631)
      at o (node_modules/plotly.js-basic-dist/plotly-basic.js:7:682)
      at Object._dereq_ (node_modules/plotly.js-basic-dist/plotly-basic.js:10:11)
      at call (node_modules/plotly.js-basic-dist/plotly-basic.js:7:631)
      at o (node_modules/plotly.js-basic-dist/plotly-basic.js:7:682)
      at Object._dereq_ (node_modules/plotly.js-basic-dist/plotly-basic.js:34001:1)
      at call (node_modules/plotly.js-basic-dist/plotly-basic.js:7:631)
      at o (node_modules/plotly.js-basic-dist/plotly-basic.js:7:682)
      at Object._dereq_ (node_modules/plotly.js-basic-dist/plotly-basic.js:99:18)
      at call (node_modules/plotly.js-basic-dist/plotly-basic.js:7:631)
      at o (node_modules/plotly.js-basic-dist/plotly-basic.js:7:682)
      at Object._dereq_ (node_modules/plotly.js-basic-dist/plotly-basic.js:112:14)
      at call (node_modules/plotly.js-basic-dist/plotly-basic.js:7:631)
      at o (node_modules/plotly.js-basic-dist/plotly-basic.js:7:797)
      at node_modules/plotly.js-basic-dist/plotly-basic.js:7:367
      at f (node_modules/plotly.js-basic-dist/plotly-basic.js:7:88)
      at Object.<anonymous> (node_modules/plotly.js-basic-dist/plotly-basic.js:7:1)
      at Object.<anonymous> (src/Chart.js:2:1)
      at Object.<anonymous> (src/App.js:6:1)
      at Object.<anonymous> (src/App.test.js:4:1)
…erty of leaflet which causes the tests to crash.
@anuveyatsu anuveyatsu merged commit d971914 into master Aug 26, 2019
@anuveyatsu anuveyatsu deleted the refactor/data-views branch August 26, 2019 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants