Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port website examples to ocular gatsby website #4033

Merged
merged 22 commits into from
Jan 28, 2020
Merged

Conversation

tgorkin
Copy link
Contributor

@tgorkin tgorkin commented Dec 17, 2019

For #3775

Port over all website examples to work with the ocular gatsby system. Recreates example "wrappers" to not use redux, reworks usage of baseui in data filtering example, other miscellaneous bug and styling fixes.

…er src directory and scss files under stylesheets.
…gatsby-examples

* commit '515466ce583f1419d2b0199daa31f850d3627bb8': (76 commits)
  fix website build (#4031)
  Tweak to scenegraph layer fix (#4027)
  Fix s2 layer polygon generation (#4024)
  Smooth edges in scatterplot (#4021)
  pydeck: Update for new @deck.gl/json API and add additional tes… (#4020)
  Fix ScenegraphLayer asset wait (#4025)
  v8.0-beta.1 Changelog (#4022)
  Bump dependency versions in modules, examples and docs (#4015)
  v8.0.0-beta.1
  bump loaders.gl to beta.5 (#4018)
  v8.0 beta
  pydeck: Make a single bundle for use in standalone and Jupyter rendering (#4010)
  Clean up LayerManager (#4011)
  Audit assert usages (#4012)
  React: eventManager listens to all children (#4013)
  Fix plot example labels (#4014)
  Bump loaders.gl to 2.0.0-beta (#4009)
  TextLayer: support background color (#3903)
  v8.0 what's new (#4001)
  Improve createProps perf (#4007)
  ...
@tgorkin tgorkin added this to the 8.1 milestone Dec 17, 2019
@coveralls
Copy link

coveralls commented Dec 17, 2019

Coverage Status

Coverage increased (+2.5%) to 83.37% when pulling a3167ce on tgorkin/gatsby-examples into b8d9df6 on master.

@ibgreen
Copy link
Collaborator

ibgreen commented Dec 17, 2019

This is awesome. Was it intentional to remove redux? You can use redux with gatsby with a fairly simple wrapComponent override see e.g. my recent Kepler.gl website-fats you folder (on master) for an example...

@tgorkin
Copy link
Contributor Author

tgorkin commented Dec 17, 2019

I did intentionally remove redux. My main reasoning was that it felt like removing redux reduced complexity and makes it easier for others to add/maintain our examples. However this may be my subjective call based on being less familiar with redux than react. I'm open to feedback here if there is a compelling case for redux.

Also, I took a look at https://github.com/keplergl/kepler.gl/blob/master/website-gatsby/src/state/redux-wrapper.js.
Good to know that its easy to add redux to a gatsby site!

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

I see a ton of "wrapper" files. It looks like a lot of code, my first question is if this could be handled in a "smarter"/less verbose way.

But perhaps these files all exist inside the existing web page, meaning they are not "new"?

@@ -4,6 +4,9 @@ import {StaticMap} from 'react-map-gl';
import DeckGL from '@deck.gl/react';
import {ScatterplotLayer} from '@deck.gl/layers';
import {DataFilterExtension} from '@deck.gl/extensions';
import {Client as Styletron} from 'styletron-engine-atomic';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This adds 10-20 lines of styletron-related code which seems like a lot of non-deck.gl-related "clutter" to add to example apps, that in my opinion should be as scaled-down as possible.

Maybe this could go into the wrapComponent gatsby callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should probably comment this change more. Firstly some necessary context: All of the apps in examples/website/* are built to be able to run standalone (by copying out the directory contents), as well as within the wrapping structure of the website. This change refactors the placement of these specific instances of the BaseUI and Styletron providers so that they are only used in the standalone case.

More fundamentally, this change moves the BaseUI and Styletron providers out of the range-input.js component and up to the root of the standalone application, which is the recommended setup as described in the BaseUI docs https://baseweb.design/getting-started/setup/. Prior to this, these root-level providers were implemented down inside of the range-input.js component. Besides going against the recommend setup, this was causing issues when this example app was plugged into the new ocular-gatsby website, due to the fact that the application root of the ocular-gatsby site already had BaseUI and Styletron providers and duplicative providers was causing errors. I've moved the clutter up from the actual UI component to the root of the application, but I think its necessary and correct in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this change moves the BaseUI and Styletron providers out of the range-input.js component and up to the root of the standalone application

OK. Sounds like overall an improvement then. I am no expert on the styling libraries or the way they have been used in the deck.gl website.

I think what triggered my comment is that I saw only one example changed here. I assume the other examples do not have this code, and that it was just that one example that was using styletron providers inside a component so you moved them up, which is fine in that case (perhaps as a first step).

import {Client as Styletron} from 'styletron-engine-atomic';
import {Provider as StyletronProvider} from 'styletron-react';
import {LightTheme, BaseProvider, styled} from 'baseui';
import {styled} from 'baseui';
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit weird to switch between styletron and styled in the examples. Should we choose one? Maybe styled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, baseui styled is a re-exported version of Styletron's styled (see https://baseweb.design/components/styled/). This change doesn't switch to a different styling system, it just moves the provider setup out of the UI component up to the root of the application (see my above comment).

@tgorkin
Copy link
Contributor Author

tgorkin commented Dec 17, 2019

In response to the proliferation of wrapper files, these are all refactored replacements for pre-existing wrappers here: https://github.com/uber/deck.gl/tree/master/website/src/components/demos

My refactoring (in my view) replaces hard-to-follow logic and usage of static functions with more self-contained explicit react component state and JSX.

@tgorkin
Copy link
Contributor Author

tgorkin commented Dec 17, 2019

Also for reference, a previous PR that contained some of the foundational setup for this new pattern of examples - https://github.com/uber/deck.gl/pull/3899/files

@ibgreen
Copy link
Collaborator

ibgreen commented Dec 17, 2019

My refactoring (in my view) replaces hard-to-follow logic and usage of static functions with more self-contained explicit react component state and JSX.

Sounds good. Though a lot of code, it looks well organized, should make things easier to maintain!

* master:
  v8.0.0-beta.2
  Layer bug fixes (#4040)
  Fix WebGL BlendEquation warnings (#4037)
  Support preprojection in PolygonTesselator (#4035)
  [Fix]: CPU Aggregation: filter out points outside of viewport. (#4026)
  React module fixes (#4032)
  Bump math.gl and probe.gl dependencies (#4029)
  RFC: Mobile Platform Support (#3999)
  Fix playground examples (#4030)
Remove unused example wrappers.
Bump node-sass version to fix errors with node v12.
* master: (82 commits)
  fix typo in performance.md
  pydeck: ArcLayer, BitmapLayer, ColumnLayer examples (#4189)
  [React] fix missing key error (#4193)
  [Bug] Fix hexagon layer projection (#4173)
  Remove HtmlWebpackPlugin from examples/playground (#4178)
  @deck.gl/json: Fix bug dropping props with falsy values (#4185)
  Fix buffer size check in Attribute.updateBuffer (#4190)
  Bump luma dependency (#4191)
  data-filter: support double precision (#4163)
  Use int type for enum uniforms (#4171)
  [TileLayer] fix tile indices generation in edge cases (#4170)
  v8.1.0-alpha.1
  Voodoo fix for Mac+NVIDIA bug (#4166)
  Remove unnecessary code from project glsl (#4162)
  Fix H3HexagonLayer update when viewport jumps (#4158)
  Refactor render tests; use stricter pass criteria (#4157)
  [Extension] Add source_target to brushing mode (#4150)
  Add offset feature to PathStyleExtension (#4126)
  Project module: support pre-projected positions (#4140)
  Repeat maps at low zoom levels (#4105)
  ...
@tgorkin tgorkin merged commit c753014 into master Jan 28, 2020
@tgorkin tgorkin deleted the tgorkin/gatsby-examples branch January 28, 2020 16:43
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.

4 participants