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

Query-builder conditions Drag&Drop functionality (take 2 - no chip gaps) #15122

Open
wants to merge 128 commits into
base: dmdimitrov/query-builder-improvements
Choose a base branch
from

Conversation

ivanvpetrov
Copy link
Contributor

@ivanvpetrov ivanvpetrov commented Nov 28, 2024

The difference between this PR and #15102 is that here component gaps are replaced with padding so we don't have void space between chips and the drag&drop works perfectly
Closes #14642

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

@teodosiah
Copy link
Contributor

'Where' should not be displayed in the grid advanced filtering
image

@gedinakova
Copy link
Contributor

In the sub-query, the search value null text should be "sub-query-results".

image

@ig-georgeA
Copy link

Can we change the edit chip mode to use 12px instead 16px? It will kinda match left and right padding on the chip.
Also, in Bootstrap And and Or casing seems off. May be how it's in code. It's not noticeable for Material and Indigo coz the button texts have a text-transform to capitalize.

12px
image

16px
image

@gedinakova
Copy link
Contributor

gedinakova commented Jan 30, 2025

@teodosiah An error is thrown when attempting to add a new group to a sub-query

  1. Open the sub-query
  2. Add a new group
  3. Add a condition to it
  4. Attempt to commit the subquery

The following error is thrown:

image

@gedinakova
Copy link
Contributor

gedinakova commented Jan 30, 2025

@teodosiah Advanced filtering button does not show the correct number of filters applied.

image

image

@ig-georgeA
Copy link

ig-georgeA commented Jan 30, 2025

@teodosiah Advanced filtering button does not show the correct number of filters applied.

Seems to happen if filter condition is on the same column. If different cols are selected, it's showing it right
image

@desig9stein
Copy link
Contributor

desig9stein commented Jan 30, 2025

Can we change the edit chip mode to use 12px instead 16px? It will kinda match left and right padding on the chip. Also, in Bootstrap And and Or casing seems off. May be how it's in code. It's not noticeable for Material and Indigo coz the button texts have a text-transform to capitalize.

12px image

16px image

For the first question, yes, but it has to be updated in the handoff. Since it's 16px there, should I change it in the implementation to 12px?

In the Bootstrap theme, the text-transform is not set and what you see is what is coming from the localization strings. I can add a style to make it capitalize in Bootstrap, do you want me to do that?

@teodosiah
Copy link
Contributor

@teodosiah Advanced filtering button does not show the correct number of filters applied.

image

image

This is the current behavior on production as well - the number is the number of columns not filters, and after checking the code as well I would say that this is by design

@sbayreva
Copy link

Material Light

  1. These labels should be grays 700
Screenshot 2025-01-31 at 9 22 56
  1. In the design, in the chip, there is 24px container for the icons which is 18px. Here we have just the icon, without the container. Also in the design the icons are black and here they are grays 900
    Screenshot 2025-01-30 at 13 36 20

  2. In the design the operators are also black and the value is within quotation marks:
    Screenshot 2025-01-30 at 13 51 59
    In the implementation their color is grays 900 with 0.6 opacity:
    Screenshot 2025-01-30 at 13 45 22
    @andiesm813 and @ig-georgeA , please can you give more details according the chip situation.

  3. Top and bottom padding here should be 12px, not 16px.
    Screenshot 2025-01-30 at 14 25 40

  4. The responsiveness is a little bit strange. Only some of the select fields are responsive. Also, if the idea is that only some of them to be responsive, and some to stay the same size, the implemented sizes does not correspond to those in the design.

Screen.Recording.mov
  1. I do not see the action buttons of the whole query builder, only in the advance filtering. (This is relevant for all themes)
    Screenshot 2025-01-30 at 14 41 06
  2. I don’t see these icons for the operators in the design. ( Relevant for all themes)
    Screenshot 2025-01-30 at 14 41 43
  3. The right and left bottom radius should be 4px
    Screenshot 2025-01-30 at 15 23 15

Advanced filtering for all themes:

The buttons in the footer change their size when you change the size of the grid. They should stay Medium for all themes in all sizes, since the QB does not support sizes.

Screenshot 2025-01-31 at 8 55 37

Material Dark

  1. The color of the labels should be grays 700
Screenshot 2025-01-31 at 9 32 39 2.The chip is different than the design: Implemented: ![Screenshot 2025-01-30 at 14 51 58](https://github.com/user-attachments/assets/beff00b3-2f89-46f1-b98f-5c520b0d50ad)

Design:
Screenshot 2025-01-30 at 14 52 29
The foreground + the icons in the design are black. The icons are in an additional container 24px in the design. In the design the value is in quotation marks.
@andiesm813 and @ig-georgeA , please can you give more details according the chip situation.

  1. Bottom left and right radius of the whole container should be 4px.
    Screenshot 2025-01-30 at 14 59 47

@andiesm813 and @ig-georgeA , please take a look at my comments.

@sbayreva
Copy link

Bootstrap Light

  1. The background of the QB in the implementation is surface 50 which is white, but the background used in the design is surface which color is actually #F8F9FA
  2. The bottom left and right radius of the whole QB should be 4px
    Screenshot 2025-01-30 at 15 44 01
  3. The height of the select and combo should be 38px, which is medium size. In the implementation the size is different:
    Screenshot 2025-01-30 at 16 19 21
  4. These labels should be grays 700
    Screenshot 2025-01-30 at 15 45 50
  5. Bottom and top paddings should be 12px
    Screenshot 2025-01-30 at 15 46 31
  6. The chips in the designs look like this:
    Screenshot 2025-01-30 at 15 47 20
    The chips in the implementation look like this:
    Screenshot 2025-01-30 at 15 48 19
    @andiesm813 and @ig-georgeA can you please give some more details according the chip. This is relevant for all themes.
  7. And and Or buttons should start with capital letter.
    Screenshot 2025-01-30 at 15 54 10

Bootstrap Dark

  1. These labels should be grays 700
    Screenshot 2025-01-30 at 16 05 54
  2. The same as for the light theme:
    Implementation:
    Screenshot 2025-01-30 at 16 10 10

Design:
Screenshot 2025-01-30 at 16 12 22

@andiesm813 and @ig-georgeA , please take a look at my comments.

@sbayreva
Copy link

sbayreva commented Jan 31, 2025

Fluent Light

  1. The background color of the whole QB should be surface , now it is white.
  2. The radiuses of the whole container should be 2px:
    Screenshot 2025-01-30 at 16 28 59
  3. The colors of the labels should be grays 700
    Screenshot 2025-01-30 at 16 33 26
  4. These buttons should be in small size:
    Screenshot 2025-01-30 at 16 35 02
  5. The background here should be surface:
    Screenshot 2025-01-30 at 16 39 38
  6. The same as in the rest of the themes. The chip is not custom as in the design.
    Implementation:
    Screenshot 2025-01-30 at 16 43 30
    Design:
    Screenshot 2025-01-30 at 16 44 44
    @andiesm813 and @ig-georgeA , please give some more details according the chip situation.
  7. The bottom and top padding here should be 12px
    Screenshot 2025-01-30 at 16 50 02

Fluent Dark

  1. These labels should be grays 700:
    Screenshot 2025-01-30 at 16 55 08
  2. The same with the chip as in rest of the themes. Implemented:
    Screenshot 2025-01-30 at 16 57 13

Design:
Screenshot 2025-01-30 at 16 57 55

@andiesm813 and @ig-georgeA , please take a look at my comments.

@sbayreva
Copy link

sbayreva commented Jan 31, 2025

Indigo Light

  1. Border radius of the whole container for the 4 radiuses should be 10px
    Screenshot 2025-01-30 at 17 12 20
  2. These labels should be grays 700
    Screenshot 2025-01-30 at 17 10 23
    3.The bottom and top paddings here should be 12px
    Screenshot 2025-01-30 at 17 17 49
  3. The problem with the chip is the same as in the rest of the themes.

Indigo Dark

  1. These labels should be grays 700
    Screenshot 2025-01-30 at 17 30 21

  2. The color of the sub query background should be white with 5% opacity
    Screenshot 2025-01-30 at 17 32 43
    Screenshot 2025-01-30 at 17 36 29

  3. The background here should be grays 50:
    Screenshot 2025-01-30 at 17 34 34
    Screenshot 2025-01-30 at 17 36 11

  4. The border of the whole sub query container should be grays 100:
    Screenshot 2025-01-30 at 17 37 18

@andiesm813 and @ig-georgeA , please take a look at my comments.

@teodosiah
Copy link
Contributor

@sbayreva, regarding the action buttons - they are part of the Grid Advanced Filtering dialog, not the Query Builder component, so this is expected:
image

@desig9stein
Copy link
Contributor

2. Also in the design the icons are black and here they are grays 900

Chip icons and text are black, they use a contrast color of gray 300 which is black, the operators use the same black color with opacity, @ig-george told me not to change anything inside the chip in this comment
https://www.figma.com/design/tuNBAEBODwLB7xxaEAvSsE?node-id=21698-56791&m=dev#1107340242

ivanvpetrov and others added 5 commits January 31, 2025 11:37
- fix query builder border radius
- fix padding on the subquery header
- fix labels colors
- fix bootstrap condition buttons text transform
- fix colors
- fix min width of the fields that are outside of subquery header
- fix button size in filtering dialog to be medium in all size variants
- cleanup unused code
* feat(query-builder): focus last edited expression chip
---------

Co-authored-by: INFRAGISTICS\IPetrov <[email protected]>
@sbayreva
Copy link

Material

  1. The background color of the hover state should be with 8% opacity, not 6%, this is for light and dark theme for the purple and the green buttons.

  2. The paddings of the ghost element looks smaller than 32px (left and right). Also, in the design the label says “drop here to insert” . Please, check the rest of the themes too, and check the designs in Figma.

  3. The label and the border should be grays 600, and there is no fill color.

  4. Please, check the design in Figma for the dark theme as well.
    Screenshot 2025-01-31 at 16 08 08

  5. The buttons for all themes should be Outlined, not Flat.
    Screenshot 2025-01-31 at 16 09 40

Bootstrap

  1. The height of the input fields should be 38px, which is medium size. In the implementation the size is different:
    Screenshot 2025-01-30 at 16 19 21

- make sure that the icon buttons are outlined
- change the size of the fields in bootstrap
- change the label inside the ghost drop indicator
- change the padding inside the ghost drop
@desig9stein
Copy link
Contributor

desig9stein commented Jan 31, 2025

Material
The buttons for all themes should be Outlined, not Flat. there was a bug logged

Here is the bug
#15299

  1. The label and the border should be grays 600, and there is no fill color.

They are the background is transparent
image

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.

9 participants