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

Allow loading query from inside of query builder #1436

Merged
merged 1 commit into from Sep 13, 2022

Conversation

YannanGao-gs
Copy link
Contributor

@YannanGao-gs YannanGao-gs commented Aug 24, 2022

Summary

Fixes #1435

How did you test this change?

  • Test(s) added
  • Manual testing (please provide screenshots/recordings)
  • No testing (please provide an explanation)

loading queries

Screen.Recording.2022-09-12.at.10.07.00.PM.mov
Screen.Recording.2022-09-12.at.5.17.03.PM.mov

No Query to Review

Screen Shot 2022-09-12 at 5 19 12 PM

@changeset-bot
Copy link

changeset-bot bot commented Aug 24, 2022

🦋 Changeset detected

Latest commit: c3c02cb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@finos/legend-application-query Minor
@finos/legend-application-query-bootstrap Patch
@finos/legend-extension-application-studio-query-builder Patch
@finos/legend-extension-dsl-data-space Patch
@finos/legend-application-query-deployment Patch
@finos/legend-application-studio-bootstrap Patch
@finos/legend-application-taxonomy-bootstrap Patch
@finos/legend-application-taxonomy Patch
@finos/legend-manual-tests Patch
@finos/legend-application-studio-deployment Patch
@finos/legend-application-taxonomy-deployment Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@finos-cla-bot finos-cla-bot bot added the cla-present CLA Signed label Aug 24, 2022
@YannanGao-gs YannanGao-gs marked this pull request as draft August 24, 2022 19:48
@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #1436 (bed9cbb) into master (91f03af) will decrease coverage by 0.03%.
The diff coverage is 19.27%.

❗ Current head bed9cbb differs from pull request most recent head c3c02cb. Consider uploading reports for the commit c3c02cb to get more accurate results

@@            Coverage Diff             @@
##           master    #1436      +/-   ##
==========================================
- Coverage   40.60%   40.57%   -0.04%     
==========================================
  Files        1230     1230              
  Lines       54176    54259      +83     
  Branches    12261    12283      +22     
==========================================
+ Hits        21997    22013      +16     
- Misses      32102    32169      +67     
  Partials       77       77              
Impacted Files Coverage Δ
...d-application-query/src/components/QueryEditor.tsx 38.46% <12.50%> (-14.54%) ⬇️
...d-application-query/src/stores/QueryEditorStore.ts 25.43% <33.33%> (+1.06%) ⬆️

Copy link
Member

@MauricioUyaguari MauricioUyaguari left a comment

Choose a reason for hiding this comment

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

small comments. Looks good. Thanks

@akphi akphi removed this from the 8.0.0 milestone Aug 30, 2022
@akphi
Copy link
Contributor

akphi commented Aug 30, 2022

@YannanGao-gs If you already linked this PR to an issue which is already milestoned (8.0.0 in this case), let's not milestone the PR, this creates duplicates in our project board. Thanks!

@YannanGao-gs YannanGao-gs force-pushed the loadQuery branch 3 times, most recently from c12b519 to 962d98a Compare September 7, 2022 15:17
Copy link
Contributor

@akphi akphi left a comment

Choose a reason for hiding this comment

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

I haven't gone through the styling code yet, one comment is that we should add some border to the table (the main display area). Here's my review so far

.changeset/giant-coats-lay.md Outdated Show resolved Hide resolved
Copy link
Contributor

@akphi akphi left a comment

Choose a reason for hiding this comment

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

Nice attempt to approach the problem from scratch, but you haven't accounted for some minor details like debouncing, let's mimic what we do in EditExistingQuerySetup and revise this PR. Thanks!

@akphi akphi merged commit df81046 into finos:master Sep 13, 2022
@akphi akphi deleted the loadQuery branch September 13, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-present CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Allow loading query from inside of query builder
4 participants