Stop defaulting values to derived property/filter/post-filter panel #2320
Conversation
🦋 Changeset detectedLatest commit: e74f76e The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
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 |
Codecov Report
@@ Coverage Diff @@
## master #2320 +/- ##
==========================================
- Coverage 40.63% 40.60% -0.04%
==========================================
Files 1539 1539
Lines 72764 73220 +456
Branches 17049 17205 +156
==========================================
+ Hits 29569 29731 +162
- Misses 43076 43369 +293
- Partials 119 120 +1
|
There was a problem hiding this comment.
There are three points:
- In terms of the interface:
- For derived property, we should warn when people are in the modal about required parameters which are not initialized
- For warning in general, we should do what we did with duplicated projection column name, there's a red (X) circle icon and the whole input is bordered in red. Consistency is key here
- There are subtleties and details we can get into, for example: initially, when no value provided, it should show red border, when people click on it to edit, the border is blue, the error icon is dismissed
-
There are other places we should check if we need this: what about calendar? Remember that we have to select a property, if the property is not selected, can we have the same interface that warns about this error?
-
You have done the check in the form (when people put in the value), in the state when we first create the filter condition, etc. but we lack this validation in the build-query-from-state flow (a.k.a. the backflow of the roundtrip), essentially, this happens when we call
QueryBuilderState.buildQuery()when we build the lambda/query; ideally, I think we should throw when we encounter invalidInstanceValuewhen building the expression, but then, we have totry ... catchall the places which usebuildQuery()and also make sure all the UI code path which potentially callsbuildQuery()is guarded properly when the query is invalid, this increases the scope of this PR, but I think it must be done
...c/stores/fetch-structure/tds/post-filter/operators/QueryBuilderPostFilterOperator_Contain.ts
Outdated
Show resolved
Hide resolved
packages/legend-query-builder/src/components/shared/BasicValueSpecificationEditor.tsx
Outdated
Show resolved
Hide resolved
packages/legend-query-builder/src/components/shared/BasicValueSpecificationEditor.tsx
Outdated
Show resolved
Hide resolved
| if (node instanceof QueryBuilderFilterTreeConditionNodeData) { | ||
| if ( | ||
| node.condition.value instanceof InstanceValue && | ||
| !isValidInstanceValue(node.condition.value) |
There was a problem hiding this comment.
I think these validation utils method can be named better, clearly, they are used only to check if the value is empty or not, if that's the case, rename them so, if we keep this name, we make them generic validator, that spits out validation issues rather than just a boolean
| @@ -157,28 +153,11 @@ export const generateValueSpecificationForParameter = ( | |||
| ), | |||
| ), | |||
| ); | |||
There was a problem hiding this comment.
strange change here, why delete these blocks?
There was a problem hiding this comment.
I deleted these blocks so we don't add default values to the instanceValues
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Summary
closes #2318
How did you test this change?
Filter Panel
filter.mp4
Derived property panel
derived.mp4