Skip to content

Conversation

@igdmdimitrov
Copy link
Contributor

@igdmdimitrov igdmdimitrov commented Aug 19, 2024

Closes #14642
Closes #14979
Closes #15389

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@igdmdimitrov igdmdimitrov added the 🛠️ status: in-development Issues and PRs with active development on them label Aug 19, 2024
@gedinakova

This comment was marked as outdated.

@gedinakova

This comment was marked as outdated.

@gedinakova

This comment was marked as outdated.

@gedinakova

This comment was marked as outdated.

@gedinakova

This comment was marked as outdated.

@Otixa

This comment was marked as outdated.

@teodosiah

This comment was marked as outdated.

@teodosiah

This comment was marked as outdated.

@teodosiah

This comment was marked as outdated.

@teodosiah

This comment was marked as outdated.

@teodosiah

This comment was marked as outdated.

@gedinakova

This comment was marked as outdated.

@gedinakova

This comment was marked as outdated.

@Otixa Otixa force-pushed the dmdimitrov/query-builder-improvements branch from 36e04c1 to 28a92c3 Compare August 29, 2024 12:06
@gedinakova
Copy link
Contributor

@desig9stein

  1. Run the dev demo with indigo theme
  2. Expand "Select return fields" combo

Result: The search & select all are not aligned with the actual items.
image

Expected result: All elements should be aligned.

@gedinakova
Copy link
Contributor

gedinakova commented Aug 29, 2024

@desig9stein

  1. Run the dev demo with bootstrap or fluent theme
  2. Put one of the conditions in edit mode
  3. Note the commit & discard buttons alignment

Result:
image
image

Expected result: The buttons should be aligned with the inputs.

@gedinakova
Copy link
Contributor

  1. Run the dev demo
  2. Clear all conditions
  3. Start a new group
  4. Add a new condition without a value
  5. Click the line group indicator

Result: The changes are discarded.

@desig9stein
Copy link
Contributor

desig9stein commented Aug 29, 2024

@desig9stein

  1. Run the dev demo with bootstrap or fluent theme
  2. Put one of the conditions in edit mode
  3. Note the commit & discard buttons alignment

Result: image image

Expected result: The buttons should be aligned with the inputs.

@AnjiManova Since this is by design, I am unsure how to proceed. The problem is that those are icon buttons, and they are big, even in small variant.

Maybe we can use just icons instead? We already have just icons here
image

@AnjiManova
Copy link

@desig9stein

  1. Run the dev demo with bootstrap or fluent theme
  2. Put one of the conditions in edit mode
  3. Note the commit & discard buttons alignment

Result: image image
Expected result: The buttons should be aligned with the inputs.

@AnjiManova Since this is by design, I am unsure how to proceed. The problem is that those are icon buttons, and they are big, even in small variant.

Maybe we can use just icons instead? We already have just icons here image

Can we just try to apply align-item: flex-end and align the buttons to the bottom instead of changing the used components?
tmp_aeda9c61-73d7-4571-92e8-c1977b10acbb

@desig9stein
Copy link
Contributor

desig9stein commented Aug 29, 2024

@desig9stein

  1. Run the dev demo with bootstrap or fluent theme
  2. Put one of the conditions in edit mode
  3. Note the commit & discard buttons alignment

Result: image image
Expected result: The buttons should be aligned with the inputs.

@AnjiManova Since this is by design, I am unsure how to proceed. The problem is that those are icon buttons, and they are big, even in small variant.
Maybe we can use just icons instead? We already have just icons here image

Can we just try to apply align-item: flex-end and align the buttons to the bottom instead of changing the used components? tmp_aeda9c61-73d7-4571-92e8-c1977b10acbb

Won't work, since the buttons are very big and there are labels on top of the inputs. the only way to align the buttons with the inputs is to use icons, I can try to make the buttons absolute and play with the position, but i don't like this idea
image

absolute position
image

Or maybe we can override the size of the buttons to match the size of the inputs in the context of the query builder

@teodosiah
Copy link
Contributor

  1. Start adding new group
  2. Type some values in the inputs
  3. Change the selected entity
  4. Confirm the change

Result: The operator and value inputs are not reset and the commit button is not disabled
QB_issue

Expected result: All inputs in the condition should be cleared and the commit button should be disabled

@AnjiManova
Copy link

@desig9stein

  1. Run the dev demo with bootstrap or fluent theme
  2. Put one of the conditions in edit mode
  3. Note the commit & discard buttons alignment

Result: image image
Expected result: The buttons should be aligned with the inputs.

@AnjiManova Since this is by design, I am unsure how to proceed. The problem is that those are icon buttons, and they are big, even in small variant.
Maybe we can use just icons instead? We already have just icons here image

Can we just try to apply align-item: flex-end and align the buttons to the bottom instead of changing the used components? tmp_aeda9c61-73d7-4571-92e8-c1977b10acbb

Won't work, since the buttons are very big and there are labels on top of the inputs. the only way to align the buttons with the inputs is to use icons, I can try to make the buttons absolute and play with the position, but i don't like this idea image

absolute position image

Or maybe we can override the size of the buttons to match the size of the inputs in the context of the query builder

If it's up to me I will not override the size of the buttons. The problem is only in the Large size in Bootstrap and in the Small size in Fluent and it is because of the fixed size of the Input Group components (Small size) in the Advanced Filtering and the Query Builder. We could consider future improvment here.

About the buttons misalignment we will discuss your concerns with the team and we will come back to you, but for now it can stay as it is.

In addition we added visual examples in all sizes for the Query Builder for all themes in the Figma file.

@ivanvpetrov
Copy link
Contributor

  1. Open Query Builder demo page
  2. Select both available conditions
  3. Group the as new And/Or group
  4. Repeat 2 and 3 again
  5. The Select expression disappears - issue
  6. Now start creating new select expression
  7. Cancel the creation.
  8. The two conditions disappear - issue
QueryBuilder_bug1.mp4

@gedinakova
Copy link
Contributor

  1. Run the dev demo
  2. Put the inner query in edit mode
  3. Click the Or Group button to add a group at the bottom
  4. Enter a valid condition and submit it
  5. Attempt to add a new condition to the newly added group

Result: The + button does not show up on the group.

Expected: One should be allowed to add new conditions to groups.

@gedinakova
Copy link
Contributor

gedinakova commented Sep 3, 2024

  1. Run the dev demo
  2. Put the 'Id equals 123' condition in edit mode (or create a new inner query)
  3. Change 'Equals' to 'In' > the value input gets disabled
  4. Add a valid condition in the inner query & submit it
  5. Commit the parent

Result: The condition remains in edit mode although the parent condition's commit button gets enabled. On attempting to commit the parent the following error is thrown:
ERROR TypeError: Cannot read properties of null (reading 'name')
at _a150.createExpressionGroupItem (query-builder-tree.component.ts:1305:64)
at _a150.init (query-builder-tree.component.ts:1526:31)
at set expressionTree (query-builder-tree.component.ts:221:14)
at applyValueToInputField (core.mjs:4045:29)
at writeToDirectiveInput (core.mjs:11443:13)
at setInputsForProperty (core.mjs:12749:9)
at elementPropertyInternal (core.mjs:12050:9)

Expected result: Adding an In/Not condition should be working fine.

@gedinakova
Copy link
Contributor

  1. Run the dev demo
  2. Clear all conditions and start a new group > all add buttons are disabled
  3. Select an entity > all add buttons get enabled
  4. Click "+ condition" button > Two empty conditions are in view
  5. Select a field in the first one

Result: Both conditions get updated with the newly selected field.
Expected result: The add buttons should not be enabled before the first condition is submitted.

@teodosiah
Copy link
Contributor

  1. Run the dev demo

    1. Put the 'Id equals 123' condition in edit mode (or create a new inner query)

    2. Change 'Equals' to 'In' > the value input gets disabled

    3. Add a valid condition in the inner query & submit it

    4. Commit the parent

Result: The condition remains in edit mode although the parent condition's commit button gets enabled. On attempting to commit the parent the following error is thrown: ERROR TypeError: Cannot read properties of null (reading 'name') at _a150.createExpressionGroupItem (query-builder-tree.component.ts:1305:64) at _a150.init (query-builder-tree.component.ts:1526:31) at set expressionTree (query-builder-tree.component.ts:221:14) at applyValueToInputField (core.mjs:4045:29) at writeToDirectiveInput (core.mjs:11443:13) at setInputsForProperty (core.mjs:12749:9) at elementPropertyInternal (core.mjs:12050:9)

Expected result: Adding an In/Not condition should be working fine.

The issue could be reproduced when there is no initially set expressionTree and a new group is added as well:
QB_issue3

@teodosiah
Copy link
Contributor

teodosiah commented Sep 4, 2024

  1. Add new group

Result:
image

Еxpected behavior:
image

  1. Commit the condition
    Result:
    image

Expected result:
image

  1. Add second condition
    Result:
    image

Expected result:
image

@igdmdimitrov
Copy link
Contributor Author

@desig9stein , please add background to 'Select All' item (when selected) and align the item:
image
image

- Remove qb overriders from igx-dialog theme,
- Remove unused variables related to the qb from igx-grid theme
- fix typography
- make sure that creating a global button theme will not override the and and or buttons
Copy link
Member

@damyanpetev damyanpetev left a comment

Choose a reason for hiding this comment

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

Leaving some comments, most of those can be addressed later (thus non-blocking for now)

Comment on lines 42 to 54
/**
* Returns an array of names of the conditions which are visible in the UI
* Returns an array of names of the conditions which are visible in the filtering UI
*/
public conditionList(): string[] {
return this.operations.filter(f => !f.hidden && !f.isNestedQuery).map((element) => element.name);
}

/**
* Returns an array of names of the conditions which are visible in the UI, including "In" and "Not In", allowing the creation of sub-queries.
*/
public extendedConditionList(): string[] {
return this.operations.filter(f => !f.hidden).map((element) => element.name);
}
Copy link
Member

Choose a reason for hiding this comment

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

Since IgxFilteringOperand is very much public (customers are advised to extend for custom scenarios), I'm not sure about this change. At least would consider a middle ground that doesn't disturb the API (like a flag to include queries), but also operations are also public so it's super simple to access externally and our internal utils really don't belong in public user configs.
Quite similar to the find methods of the expression tree IIRC and perhaps worth of similar fate (to be phased out).

* @hidden
*/
protected findValueInSet(target: any, searchVal: Set<any>) {
public findValueInSet(target: any, searchVal: Set<any>) {
Copy link
Member

Choose a reason for hiding this comment

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

Also not sure why this became public, can't see any external use and we've already got enough of those visible :)

Comment on lines +116 to +118
}];

this.operations = newOperations.concat(this.operations);
Copy link
Member

Choose a reason for hiding this comment

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

If we're changing this wouldn't mind also just ...this.operations and the end. With those array sizes and this running once, it'll negligible diff, if any :)

Copy link
Member

Choose a reason for hiding this comment

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

Also empty line with indents

Comment on lines +194 to +199

/**
* @hidden @internal
*/
public generateEntity() {
const entities = [
Copy link
Member

Choose a reason for hiding this comment

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

Don't need any more of those semi-public things for templates anymore

Suggested change
/**
* @hidden @internal
*/
public generateEntity() {
const entities = [
protected generateEntity() {
const entities = [

Also, and I can't say the old code was any better in this regard, not really the best option to have a function generate this array basically on every change detection. Pretty much guarantees whatever logic is behind entities is ran too. Both the filter and entity transform are probably best done by a pipe.

}

public set searchVal(value: any) {
this.expressionUI.expression.searchVal = value ? new Date(Date.parse(value.toString())) : null;
Copy link
Member

Choose a reason for hiding this comment

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

Ough, always a bit sketchy seeing a direct Date.parse/new Date on what I'm assuming is user data directly? Those tend to have oddities with both formats against locales and time offsets with date-only ISO. For examples: our own internal DateUtil has such parseISODate and you can have a look behind Angular's toDate which is used for formatDate/DatePipe

Regardless, would love if not every small grid component does its own parsing however, but have a single util that definitely supports what we need (eventually I guess). At the very least, I see some of the filtering strategy going to core/utils parseDate so we can at least try to move towards standardizing on that for the Grids?

Comment on lines 169 to +173
/**
* @hidden @internal
*/
public fieldSelectOverlaySettings: OverlaySettings = {
scrollStrategy: new AbsoluteScrollStrategy(),
modal: false,
closeOnOutsideClick: false
};
@ContentChild(IgxQueryBuilderSearchValueTemplateDirective, { read: TemplateRef })
public searchValueTemplate: TemplateRef<any>;
Copy link
Member

Choose a reason for hiding this comment

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

Again, would prefer if these props were directly private/protected

this.deselectParentRecursive(parent);
}
}
const inIcon = '<svg xmlns="http://www.w3.org/2000/svg" height="24px" viewBox="0 -960 960 960" width="24px" fill="#5f6368"><path d="M560-280H120v-400h720v120h-80v-40H200v240h360v80Zm-360-80v-240 240Zm560 200v-120H640v-80h120v-120h80v120h120v80H840v120h-80Z"/></svg>';
Copy link
Member

Choose a reason for hiding this comment

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

Are those supposed to go into the actual package at some point?

Comment on lines -44 to -45
igx_query_builder_create_and_group?: string;
igx_query_builder_create_or_group?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Removing those is technically a breaking change, however small. The resource interfaces are public API, just sayin :)

Copy link
Contributor

Choose a reason for hiding this comment

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

added migration in this PR - #15381

simeonoff
simeonoff previously approved these changes Feb 24, 2025
* fix(*): Renamed nested query condtions
ChronosSF
ChronosSF previously approved these changes Feb 25, 2025
…ader title (#15381)

* feat(query-builder): expose disableReturnFieldsChange prop on root level
* feat(query-builder): hide header, remove title RS, update CHANGELOG and README.md
* chore(*): Move header variants down in the dev demo.

---------

Co-authored-by: igdmdimitrov <[email protected]>
Co-authored-by: desig9stein <[email protected]>
Co-authored-by: Galina Edinakova <[email protected]>
@damyanpetev damyanpetev merged commit 16d2f95 into master Feb 25, 2025
5 checks passed
@damyanpetev damyanpetev deleted the dmdimitrov/query-builder-improvements branch February 25, 2025 15:55
@damyanpetev damyanpetev mentioned this pull request Mar 4, 2025
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chip: long suffix and prefix wrap on 2 lines Query Builder component update Query Builder: Update theme to match latest design