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

Map: Update 'Markers' menu entry to allow more advanced user configuration #190

Closed
13 of 14 tasks
d-callan opened this issue May 10, 2023 · 8 comments · Fixed by #285
Closed
13 of 14 tasks

Map: Update 'Markers' menu entry to allow more advanced user configuration #190

d-callan opened this issue May 10, 2023 · 8 comments · Fixed by #285
Assignees

Comments

@d-callan
Copy link
Contributor

d-callan commented May 10, 2023

Additional tweaks from 2023-06-15 Map UX meeting:

  • try to use the same layout as regular visualizations for the marker config panel:
    - variable picker at top
    - can we give this a max-width?
    - not discussed at meeting, but if it's at the very top, does it really need a "Color:" heading? (For continuous vars, is the color gradient really a color? maybe)
    - plot next (in this case it's the marker preview)
    - controls
    - value picker distributions (checkbox table or histogram)
  • Heading for preview is now "Summary marker (all filtered data)" (plus possible tooltip to explain that it's visible plus any other filtered data outside the viewport?)
  • Controls subheadings
    - chart marker: Marker X-axis controls (continuous only: contains "binning method"), Marker Y-axis controls (contains: count/proportion radio button and logscale toggle). The extra "Marker" in the headings might be overkill but let's see it first.
    - donut marker: Doesn't have an X-axis. We just need to have a binning method selector for continuous variables. Do we just use a "Donut controls" heading? Bit of a grey area...
  • Binning method selector implementation notes: the ContinousVariableMetadataResponse contains three types of binRanges: equalInterval, quantile (actually decile) and standardDeviation. We are currently hardcoded to use equalInterval somewhere. Should be simple to use the other two types based on the radio button-controlled state.
  • For categorical variables, put a box around the checkbox table (and add a scroll bar) - see figma - color up to you!
  • We didn't decide on a search box in the checkbox table - exclude for now.
  • Add a radio button control inside the box under the table, "Show counts for ⚪ filtered ⚫ visible" (categorical only)
  • I'm 95% sure we decided this radio button will NOT affect the summary marker (that's why we put the box around the table)
  • Remove the asterisk from the variable selector when the input is "non-nullable" #313
  • Continuous reference distribution
    - try labelling it "Raw distribution of over all filtered data" (tooltip could mention "equal-binned" ??)
    - color uniform changes for taxon_id link and resource linl -refs #51670 #333 (add this to HistogramProps: colorPalette: Array(8).fill('#333'))
    - reduce margins (spacingOptions should hopefully do it)

More categorical tweaks

  • debounce with 2s delay
  • confirmation of previous plans to deal with >7 selections
    - allow any number of checkbox selections
    - but only use the last valid combination of checkboxes for the overlayConfig
    - overlay a warning banner on top of the Summary marker (aka preview marker) to say "maximum 7 selections allowed, please deselect X values" (summary marker should always be visible, hence the scroll bar for the checkbox table)
  • investigate issue with summary marker containing stale data after filters have been changed (Bob can demo)

General menu-related

mockups exist in figma.. ex screenshots as well

categorical
image

continuous
image

NOTE: I believe some changes were discussed after these mockups were made, but idk if the mockups were updated to reflect them. For ex, The histogram shown when configuring a continuous variable will always reflect the subset, rather than offering the visible vs filtered toggle.

@asizemore @bobular do you know of any other changes discussed which are important to note here?

@bobular
Copy link
Member

bobular commented May 18, 2023

Some implementation thoughts for the "Choose values" sorted checkbox list component:

Assume #143 is merged or branching from it!

PieMarkerConfigurationMenu and BarPlotMarkerConfigurationMenu need to display the new component when overlayConfig.overlayType === 'categorical'

overlayConfig.overlayValues will already* contain the values that should have their checkboxes checked.

(* the updateOverlayConfig useEffect sets this via an async function when the overlay variable or filters change. @dmfalke is going to cull some of our useEffects though...)

However the checkbox table thing needs the full list of values. You can get these from subsettingClient.getDistribution() which you can see used in getMostFrequentValues() in defaultOverlayConfig.ts. I think we will pass the current filters to that request, but that's a detail that can be ironed out later, as is the viewport/whole_world radio button (which requires an extra viewport filter to be sent to the back end).

Are we OK having two different bits of code calling subsettingClient.getDistribution() or do we need to make getMostFrequentValues a bit more general purpose? Decisions decisions...

Anyway, checking the checkboxes should update the overlayConfig (changing its overlayValues) and everything else should just fall into place...!

@dmfalke
Copy link
Member

dmfalke commented May 18, 2023

Are we OK having two different bits of code calling subsettingClient.getDistribution() or do we need to make getMostFrequentValues a bit more general purpose? Decisions decisions...

Unless I'm mistaken, both uses are in relation to the overlay variable of the marker. It might make sense to pull that call out to a separate variable that gets returned by the marker hook.

@bobular
Copy link
Member

bobular commented May 18, 2023

I'm not sure I understand.

Also should we use the <Mesa> checkbox/table component for now? I know we came to that decision already but it might take Sam three days to figure out how to use it.

@asizemore
Copy link
Member

A few updates to the mockups that were discussed but are no where represented:

  • checkboxes will always be active. If a user interacts with them they get the Apply/Cancel buttons
  • Use subtitle of the section possibly to save the space for the appearing Apply/Cancel buttons

Let me know if it would be helpful to have these or other changes in mockup form!

@bobular
Copy link
Member

bobular commented May 19, 2023

Thanks @asizemore - I think that's a great text summary, probably no need for mock-ups.

@bobular
Copy link
Member

bobular commented May 19, 2023

Unless I'm mistaken, both uses are in relation to the overlay variable of the marker. It might make sense to pull that call out to a separate variable that gets returned by the marker hook.

Ah, I think I understand. When the original call is made to getDefaultOverlayConfig we should also return the full list of overlay values and their counts, so we can re-use them in the new UI. It's not currently a hook, but probably could be. Let's see how things look when Dave has exorcised the effects.

@dmfalke
Copy link
Member

dmfalke commented May 23, 2023

Unless I'm mistaken, both uses are in relation to the overlay variable of the marker. It might make sense to pull that call out to a separate variable that gets returned by the marker hook.

Ah, I think I understand. When the original call is made to getDefaultOverlayConfig we should also return the full list of overlay values and their counts, so we can re-use them in the new UI. It's not currently a hook, but probably could be. Let's see how things look when Dave has exorcised the effects.

Sorry for the delayed response. Yes, the basic idea is that we get the distribution once, and pass them to the config menu and the legend, one way or another. I haven't thought about the specifics of how we would do it. There may be some challenges.

@dmfalke
Copy link
Member

dmfalke commented Jun 30, 2023

Regarding debouncing, I suggest we wait and see if it's really needed. My theory is that most users will not be able to click more than one item within a reasonable debounce window.

If that proves to be wrong, there is a very nice implementation of a react-friendly debounce hiding in

// region Debouncer
/**
* Debouncer Function Type.
*
* Represents a function that may be used to build a "debounced" or "debouncing"
* function by passing in a target function that takes a value of type `T` and
* returns nothing.
*
* The return value of a Debouncer function call will be a new function that
* wraps the given target function with debouncing.
*
* @param T Type of the value consumed by the function wrapped by and returned
* by the Debouncer function.
*/
type Debouncer<T> = (func: (value: T) => void) => (value: T) => void;
/**
* Builds a new `Debouncer` function that may be used to build one or more
* functions that debounce on the same timer.
*
* @param delay Debouncing delay.
*
* @return A `Debouncer` function that may be used to build one or more
* functions that debounce on the same timer.
*/
function buildDebouncer<T>(delay: number): Debouncer<T> {
let timer: any;
return (func: (value: T) => any) => {
return (value: T) => {
clearTimeout(timer);
timer = setTimeout(() => func(value), delay);
};
};
}
// endregion Debouncer

ETA see this PR comment: #332 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants