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

Refactoring React QueryBuilder to be uncontrolled #2303

Merged

Conversation

@SteffeyDev
Copy link
Contributor

@SteffeyDev SteffeyDev commented Mar 5, 2021

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Broken off from #2196

I ran into some issues while making other changes to the React QueryBuilder, and realized that it was because the QueryBuilder was neither fully controller nor uncontrolled, and was using the React anti-pattern of unconditionally copying props to state. Following the recommendations in that article and based on my experience as a React developer, I decided to make QueryBuilder uncontrolled. This means that QueryBuilder controls it's own state, and can't be modified from the outside save for in specific ways that are allowed through props.

This involved deprecating the query, setQuery, vizState, and setVizState props, and adding the initialVizState and onVizStateChanged props. From my analysis, the most common use case for accessing the vizState outside of the query editor is to save the editor state in persistent storage. These two new props allow for setting up a QueryBuilder component from a saved state and then listening for changes to save back. I'm using this in my app to save the vizState to a database so I can persist the QueryBuilder state across reloads.

I also added the defaultQuery and defaultChartType props, so that if initialVizState is empty, it will pick up the default values for those. defaultChartType was listed in the documentation before, but wasn't actually implemented until now.

@SteffeyDev SteffeyDev requested a review from cube-js/core as a code owner Mar 5, 2021
@codecov
Copy link

@codecov codecov bot commented Mar 5, 2021

Codecov Report

Merging #2303 (4ffd9ee) into master (f52f400) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2303   +/-   ##
=======================================
  Coverage   55.74%   55.74%           
=======================================
  Files         118      118           
  Lines        8692     8692           
  Branches     1891     1891           
=======================================
  Hits         4845     4845           
  Misses       3468     3468           
  Partials      379      379           
Impacted Files Coverage Δ
packages/cubejs-api-gateway/src/gateway.ts 71.75% <0.00%> (ø)
packages/cubejs-server-core/src/core/server.ts 50.45% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f52f400...4ffd9ee. Read the comment docs.

@vasilev-alex vasilev-alex merged commit a1b4201 into cube-js:master Mar 11, 2021
17 of 18 checks passed
17 of 18 checks passed
unit (10.x)
Details
unit (12.x)
Details
unit (14.x)
Details
unit (15.x)
Details
lint lint
Details
build
Details
integration-redis (12.x)
Details
integration-wrk-1 (12.x)
Details
integration-wrk-2 (12.x)
Details
integration-wrk-3 (12.x) integration-wrk-3 (12.x)
Details
Build only :dev image
Details
Build only Alpine :dev image
Details
docker-image-latest-set-tag
Details
Build only :latest image
Details
Build only :alpine image
Details
codecov/patch Coverage not affected when comparing f52f400...4ffd9ee
Details
codecov/project 55.74% (+0.00%) compared to f52f400
Details
netlify/cubejs-docs-staging/deploy-preview Deploy preview canceled.
Details
@SteffeyDev SteffeyDev mentioned this pull request Mar 12, 2021
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants