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

feat: Add single select and inverse selection to numeric range (#16722) #17372

Merged
merged 7 commits into from
Dec 1, 2021
Merged

feat: Add single select and inverse selection to numeric range (#16722) #17372

merged 7 commits into from
Dec 1, 2021

Conversation

mhoustonataegis
Copy link
Contributor

SUMMARY

Support the ability to filter by numeric range as a single maximum, minimum, or exact value.

Filters config form BEFORE:

Before_1

Range filter plugin BEFORE:

Before_2

Filters config form AFTER:

After_1

Range filter plugin AFTER, using MAXIMUM value:

After_2

Range filter plugin AFTER, using EXACT value:

After_3

Range filter plugin AFTER, using MINIMUM value:

After_4

TESTING INSTRUCTIONS

Using a numerical range filter, choose "Single Value" in the advanced section. This will reveal 3 single value type options supporting the following behaviors:
* Minimum: Filter values greater than or equal to the selected value on the range slider.
* Exact: Filter values equal to the selected value on the range slider.
* Maximum: Filter values less than or equal to the selected value on the range slider.

If "Single Value" is not checked, the previous default range slider behavior applies.
If "Single Value" is checked and Minimum is chosen, the slider should support a single thumb sliding from the right hand side of the control.
If "Single Value" is checked and Exact is chosen, the slider should support a single thumb sliding in the middle of the control with no highlighted range displayed.
If "Single Value" is checked and Maximum is chosen, the slider should support a single thumb sliding from the left hand side of the control.

ADDITIONAL INFORMATION

@@ -0,0 +1,5 @@
export enum SingleValueType {
Copy link
Member

@geido geido Nov 9, 2021

Choose a reason for hiding this comment

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

@mhoustonataegis this file is missing the license. CI won't pass without it

Suggested change
export enum SingleValueType {
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
export enum SingleValueType {

@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Merging #17372 (01d3c71) into master (e2a429b) will decrease coverage by 8.36%.
The diff coverage is 88.14%.

❗ Current head 01d3c71 differs from pull request most recent head 8c1da02. Consider uploading reports for the commit 8c1da02 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17372      +/-   ##
==========================================
- Coverage   76.94%   68.58%   -8.37%     
==========================================
  Files        1042     1602     +560     
  Lines       56248    65331    +9083     
  Branches     7784     6989     -795     
==========================================
+ Hits        43282    44805    +1523     
- Misses      12707    18640    +5933     
- Partials      259     1886    +1627     
Flag Coverage Δ
javascript 57.14% <88.14%> (-13.88%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...uperset-ui-core/src/query/types/AnnotationLayer.ts 82.35% <ø> (ø)
...ackages/superset-ui-core/src/query/types/Column.ts 100.00% <ø> (ø)
...ges/superset-ui-core/src/query/types/Datasource.ts 100.00% <ø> (ø)
...ackages/superset-ui-core/src/query/types/Filter.ts 75.00% <ø> (ø)
...ackages/superset-ui-core/src/query/types/Metric.ts 100.00% <ø> (ø)
...kages/superset-ui-core/src/query/types/Operator.ts 83.33% <ø> (ø)
...superset-ui-core/src/query/types/PostProcessing.ts 58.33% <ø> (ø)
...packages/superset-ui-core/src/query/types/Query.ts 100.00% <ø> (ø)
.../superset-ui-core/src/query/types/QueryFormData.ts 87.50% <ø> (ø)
.../superset-ui-core/src/query/types/QueryResponse.ts 100.00% <ø> (ø)
... and 1169 more

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 e2a429b...8c1da02. Read the comment docs.

@BillGreerUX
Copy link

This PR is exactly what I requested with #16722. This will solve our use case well.

@riahk
Copy link
Contributor

riahk commented Nov 23, 2021

Code looks good, but the MIN slider UI is a bit misleading.
Screen Shot 2021-11-23 at 11 43 37 AM
Screen Shot 2021-11-23 at 11 43 43 AM

I get that the reverse flag is meant to distinguish it from the MAX slider. I dug around through the antd docs and couldn't find a decent solution, though. I'm wondering if there's a way to style the MIN slider so the colors are inverted, here? That way the blue would appropriately distinguish the included range.
Also — and this is probably excessive, since the user can name the filter whatever they want — maybe having an indicator next to the label of whether the slider is MIN/MAX/EXACT could be useful?

@villebro
Copy link
Member

I'm wondering if there's a way to style the MIN slider so the colors are inverted, here? That way the blue would appropriately distinguish the included range.

@riahk I agree, this should be doable fairly easily. To achieve an inverse selection, I simply flipped around the the background colors of ant-slider-rail and ant-slider-track on the Slider example page and the end result looks pretty ok:

image

Probably requires some additional fine tuning (the hovers need to also be updated), but in the absence of full support for this in the AntD component, this may be the most visually appealing solution for now.

Copy link
Contributor

@riahk riahk left a comment

Choose a reason for hiding this comment

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

Is inverse selection actually included in this PR? or is that somewhere else?

/>
)}
{enableSingleMinValue && (
<Slider
Copy link
Contributor

Choose a reason for hiding this comment

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

As a follow-up to my earlier comment about swapping bg colors: I played around with the styles here to give an idea of what that might look like!

const StyledMinSlider = styled(Slider)`
  .ant-slider-rail {
    background-color: #99e7f0;
  }

  .ant-slider-track {
    background-color: #f5f5f5;
  }

  &:hover {
    .ant-slider-rail {
      background-color: #99e7f0;
    }

    .ant-slider-track {
      background-color: #f5f5f5;
    }
  }
`;

Result:
Screen Shot 2021-11-24 at 10 18 07 AM
Screen Shot 2021-11-24 at 10 18 12 AM

@mhoustonataegis
Copy link
Contributor Author

To maintain the visual distinction between a min and max filter, it may make more sense to flip the values on the range rather than flipping the highlighted section.

For example, there is no visual distinction between a min and max slider if the highlighted section is flipped. Here, "Months" is a max range and "Years" is a min range, but there is no visual way to discern the difference:

image

However, with the range values flipped, the highlighted section could continue to exist on the right while also corresponding to the correct filtered values:

image

This would result in the visual distinction between a min and max slider:

image

@mhoustonataegis
Copy link
Contributor Author

Is inverse selection actually included in this PR? or is that somewhere else?

There is no explicit inverse selection supported here.

@BillGreerUX
Copy link

This would result in the visual distinction between a min and max slider:

image

This design is a better user experience. This visual pattern allows for realization of minimum and maximum. By allowing the visual distinction between min and max, there is not a need to add an indicator next to the label of whether the slider is MIN/MAX/EXACT. With the visual distinction and not textual, it will be an easier transition when moving too multilingual in future releases.

Also, the above visual pattern matches the range pattern below.

image

I feel @mhoustonataegis suggested implementation would be the best.

@riahk
Copy link
Contributor

riahk commented Nov 29, 2021

@mhoustonataegis @BillGreerUX These are excellent points! I like having the colors inverted for that distinction.

What's confusing me still is that it's not clear to me which ends of the scale are high/low. With the reverse flag on the min slider, the colors invert, but so do the position of the high/low values! i.e. the "low" value is actually on the right, and the "high" is on the left, which can be confusing.

My suggestion is to remove the reverse flag from the min slider, and then add the custom styling mentioned in my earlier comment to invert the colors. The effect is as follows:
Screen Shot 2021-11-29 at 12 39 38 PM

In this case, both sliders have the low value on the left, and the color inversion provides a distinction while still clearly indicating the inclusive range for the filter. Thoughts?

@mhoustonataegis
Copy link
Contributor Author

@mhoustonataegis @BillGreerUX These are excellent points! I like having the colors inverted for that distinction.

What's confusing me still is that it's not clear to me which ends of the scale are high/low. With the reverse flag on the min slider, the colors invert, but so do the position of the high/low values! i.e. the "low" value is actually on the right, and the "high" is on the left, which can be confusing.

My suggestion is to remove the reverse flag from the min slider, and then add the custom styling mentioned in my earlier comment to invert the colors. The effect is as follows: Screen Shot 2021-11-29 at 12 39 38 PM

In this case, both sliders have the low value on the left, and the color inversion provides a distinction while still clearly indicating the inclusive range for the filter. Thoughts?

Yes, my suggestion included removing the reverse flag, so that both min and max sliders used the same number lines for their values, but only different highlighting based upon your suggested css. So I think we're on the same page. I'll push up a new commit.

… highlighted range value accurately reflects the applied filter.
Copy link
Contributor

@riahk riahk left a comment

Choose a reason for hiding this comment

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

LGTM!

@riahk riahk merged commit 54b56fe into apache:master Dec 1, 2021
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.

6 participants