[GitHub] [superset] stevetracvc commented on pull request #18009: feat: log scale and step size control for range filters [WIP]

[GitBox <gi...@apache.org>]

stevetracvc commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1086696409


   Awesome! Questions inline:
   
   > 
   > We had a conversation with @michael-s-molina who has suggested the following:
   > 
   > * The easiest solution is just to leave it in the current position if we can live with it (of course)
   > * Another way is to move the code directly into `FiltersConfigForm` between the description and the default value
   
   If we did that, then that moves more of the config out of the specific native filter component's controlPanel file and into the main one. I had tried to make this more general so that other filter types could also include select boxes for controls if needed. If you do want the FiltersConfigForm modified instead, it seems like it should go under the "single value" checkbox, as part of the configuration rather than the settings. What do you think?
   
   > 
   > You are right this wasn't straightforward forward but we aim to improve the codebase of native filters in the upcoming phase 2.
   
   Since that's the plan, which route would be better? It seems like we'd want everything in the specific component's controlPanel rather than spread across the two files. And maybe revisit this when phase 2 is complete?
   
   > 
   > Some other observations that came out from our conversation:
   > 
   > * This functionality should be under a checkbox as it’s very specific, such as “Customise default step scaling function”
   
   Is it really that specific? Am I one of the few people using range filters for a large range of values? If you want it hidden like that, then the wording on that needs improvement, since step size and scaling can each be used independent of the other one. Eg, you can't currently use a range filter for floats since the step size is inherently 1 (especially on a range 0 to 1). I feel like each one is its own thing, and step size could be incorporated without bothering with the scaling.
   
   
   > * The inputs should not take the full width of the Modal. Please follow the same UI of existing inputs
   
   I finally found where it does its magic, so I made them half width now (I haven't pushed yet). I'm starting to understand how everything works here, but a lot of what I originally did was just get it to work because these were features I needed...so I copied and pasted code.
   
   The issue is that FiltersControlModal calls getControlItemsMap, which calls getControlItems, which **flattens** the controlPanel config array. I think ideally, step size and scaling function would be in the same row, don't you think? But that would take a decent rework of the FiltersConfigModal code to make that work.
   
   
   > * If the step size is lower than two decimals, the exact number of decimals should also be visible in the left sidebar, otherwise it’s impossible to see those decimals changing.
   
   Not sure I follow on this one, though it also might be related to the issue of values not being in exact increments of the step size. When I have the step size at 0.001, linear scale, for something with values 0 to 1, each increment is actually 0.005, which I think is a limitation on the antd slider control.
   
   > 
   > Let me know if you have any questions. Thanks for the hard work!
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org