Faceting #259 #272

chowington · 2021-11-22T23:04:10Z

Works toward #259

dmfalke · 2021-11-23T04:51:50Z

@chowington I updated the linked issue with some missing details. It may impact the work you've done here.

bobular

Hi!
Looks to me like there's a bug in the Storybook Controls UI

In Barplot, for example, I would say there should only be 2 controls: data and props, but it lists 27. 25 of them are inoperative. But I guess we shouldn't dwell on this right now.

Looks great otherwise, cheers.

bobular · 2021-12-02T13:17:02Z

Looks to me like there's a bug in the Storybook Controls UI

In Barplot, for example, I would say there should only be 2 controls: data and props, but it lists 27. 25 of them are inoperative. But I guess we shouldn't dwell on this right now.

Maybe the controls are shared per story tsx file? Or leak between stories in the same file, or something? Not worth investigating further, just thought I'd make a note of this here.

dmfalke · 2021-12-02T13:29:21Z

I took a very quick look at the code, and I think you're going in the right direction. I haven't tested anything, but hope to do so in the next day or two.

chowington · 2021-12-03T01:24:49Z

@bobular @dmfalke The main tasks for this PR's ticket are mostly done. Do take a look please! There's one outstanding bug and potentially other improvement to make that we can discuss.

bobular

This looks great. I tested also by nuking modalComponentProps in storybook controls and it works great with the modals disabled.

Am I correct in thinking that if we have N facets, we have N on-screen components that each "contain" data. We also have N lots of divModalProps.onClick handlers which also contain a copy of the data (in sharedProps). Is that using twice the memory? Or is it just a copy by reference. And even if it is a deep copy, does it even matter? (thinking scatterplots)

Excellent work with the interactive/displayLegend stuff and I think it is essential to set the modal plot's title, so all 👍 from me.

bobular · 2021-12-03T14:06:26Z

Oh there is one small thing. If you check the Histogram story, you see that the modal plot has a legend with a single item.

In EDA we disable the legend when there's only one data series. So we will want to be able to control displayLegend from the upstream component, rather than have it always true as below

          const divModalProps = modalComponentProps && {
            onClick: () => {
              setModalPlot(
                <Component
                  {...sharedProps}
                  displayLegend={true}
                  interactive={true}
                  {...modalComponentProps}
                  title={label}
                />
              );
              setModalIsOpen(true);
            },
            style: { cursor: 'pointer' },
            title: 'Click to expand',
          };

chowington · 2021-12-03T17:33:16Z

This looks great. I tested also by nuking modalComponentProps in storybook controls and it works great with the modals disabled.

Am I correct in thinking that if we have N facets, we have N on-screen components that each "contain" data. We also have N lots of divModalProps.onClick handlers which also contain a copy of the data (in sharedProps). Is that using twice the memory? Or is it just a copy by reference. And even if it is a deep copy, does it even matter? (thinking scatterplots)

Excellent work with the interactive/displayLegend stuff and I think it is essential to set the modal plot's title, so all 👍 from me.

Thanks Bob!

I believe JS's object passing behavior is a (slightly-modified) form of pass-by-reference, so theoretically we aren't duplicating the data here. Definitely a valid concern though!

chowington · 2021-12-03T17:37:15Z

Oh there is one small thing. If you check the Histogram story, you see that the modal plot has a legend with a single item.

In EDA we disable the legend when there's only one data series. So we will want to be able to control displayLegend from the upstream component, rather than have it always true as below
          const divModalProps = modalComponentProps && {
            onClick: () => {
              setModalPlot(
                <Component
                  {...sharedProps}
                  displayLegend={true}
                  interactive={true}
                  {...modalComponentProps}
                  title={label}
                />
              );
              setModalIsOpen(true);
            },
            style: { cursor: 'pointer' },
            title: 'Click to expand',
          };

This should already allow that! The current logic allows you to override displayLegend={true} by passing your own displayLegend value in modalComponentProps, as {...modalComponentProps} overwrites the previous props 👍

bobular · 2021-12-03T17:41:07Z

Oh yeah, sorry I didn't spot that!

…

On Fri, Dec 3, 2021 at 5:37 PM Connor Howington ***@***.***> wrote: Oh there is one small thing. If you check the Histogram story, you see that the modal plot has a legend with a single item. In EDA we disable the legend when there's only one data series. So we will want to be able to control displayLegend from the upstream component, rather than have it always true as below const divModalProps = modalComponentProps && { onClick: () => { setModalPlot( <Component {...sharedProps} displayLegend={true} interactive={true} {...modalComponentProps} title={label} /> ); setModalIsOpen(true); }, style: { cursor: 'pointer' }, title: 'Click to expand', }; This should already allow that! The current logic allows you to override displayLegend={true} by passing your own displayLegend value in modalComponentProps, as {...modalComponentProps} overwrites the previous props 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#272 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACLLH6YBOZJL6FL7C7XLFLUPD55NANCNFSM5ISFCPEA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

chowington · 2021-12-03T17:44:15Z

@bobular @dmfalke The main tasks for this PR's ticket are mostly done. Do take a look please! There's one outstanding bug and potentially other improvement to make that we can discuss.

Sorry, I was a little vague late last night! More details:

Bug (Fixed): Clicking directly on the pie chart portion of a pie facet doesn't trigger the click handler that opens the modal. You have to click outside the pie itself. I'm not sure why exactly this is happening yet; if anyone has an idea, let me know!
Comment: I'll probably double check the layout prop defaults to make sure they are reasonable
Question: Should we have defaults for the modal component layout props as well?
Question: How should we handle the mosaic contingency table? Should there be a modal for those tables as well? I didn't test that at all and there isn't a story for the cont table, faceted or otherwise

chowington · 2021-12-06T14:36:51Z

@dmfalke @bobular Any comments on the above?

bobular · 2021-12-07T13:24:17Z

Thanks @chowington - having a look now.

Unrelated to your questions, I saw this in the console, FYI:

error: "Warning: react-modal: App element is not defined. Please use Modal.setAppElement(el) or set appElement={el}. This is needed so screen readers don't see main content when modal is opened. It is not recommended, but you can opt-out by setting ariaHideApp={false}."

dmfalke · 2021-12-07T14:00:07Z

@bobular @dmfalke The main tasks for this PR's ticket are mostly done. Do take a look please! There's one outstanding bug and potentially other improvement to make that we can discuss.

Sorry, I was a little vague late last night! More details:
* [x]  Bug (Fixed): Clicking directly on the pie chart portion of a pie facet doesn't trigger the click handler that opens the modal. You have to click outside the pie itself. I'm not sure why exactly this is happening yet; if anyone has an idea, let me know!

* [ ]  Comment: I'll probably double check the layout prop defaults to make sure they are reasonable

* [ ]  Question: Should we have defaults for the modal component layout props as well?

I think it makes sense to have defaults.

* [ ]  Question: How should we handle the mosaic contingency table? Should there be a modal for those tables as well? I didn't test that at all and there isn't a story for the cont table, faceted or otherwise

My initial thought is to have the same set of tabs we include for an unfaceted mosaic plot.

bobular

Code looks good, works great.

It would be nice to have the mosaic modal have the table tabs too, but isn't this going to be a nightmare to implement? Instead of passing modalComponentProps to FacetedMosaicPlot we'll have to pass an actual component. Maybe that's an OK hack?

bobular · 2021-12-07T14:45:56Z

src/plots/facetedPlots/FacetedPiePlot.tsx

+  const onPlotlyRender = useCallback((_, graphDiv) => {
+    // Replace each slice DOM node with a copy of itself.
+    // This removes the existing click event handler, which
+    // otherwise blocks the click handler that opens the modal


Thanks for the comment - was looking at an older commit and was going to ask for one!

dmfalke · 2021-12-07T15:32:06Z

Code looks good, works great.

It would be nice to have the mosaic modal have the table tabs too, but isn't this going to be a nightmare to implement? Instead of passing modalComponentProps to FacetedMosaicPlot we'll have to pass an actual component. Maybe that's an OK hack?

You're right, I didn't think about that. Let's just stick with the plot for now and see what others think.

dmfalke

I think this all looks great. One suggestion is to export the default styling and spacing options so that consumers can use them if passing in custom styling or spacing.

chowington · 2021-12-07T19:27:42Z

Thanks @chowington - having a look now.

Unrelated to your questions, I saw this in the console, FYI:

error: "Warning: react-modal: App element is not defined. Please use Modal.setAppElement(el) or set appElement={el}. This is needed so screen readers don't see main content when modal is opened. It is not recommended, but you can opt-out by setting ariaHideApp={false}."

Yeah, I saw that. The modal comes from core-components, and Mike said he'll address that console warning at some point 👍

chowington · 2021-12-07T19:34:43Z

Question: Should we have defaults for the modal component layout props as well?

I think it makes sense to have defaults.

I thought about and worked on this a little bit and I think the resulting API would be a bit odd. Currently modalComponentProps existing is the way we activate the click-to-expand feature. If we provide sensible defaults, it would be odd for the user to still have to provide modalComponentProps in order to activate the click-to-expand feature.

Instead I attempted to create a better API by providing a clickToExpand?: boolean prop that controls the expand feature's activation, however after changing the logic appropriately, there was a complicated TS error that cropped up involving the P generic type. I think I understood it, but I didn't see a way to get around it.

So what all this means is that I decided to stick with what we have currently, including not giving default modalComponentsProps (I did stash my attempt at a better API though). How does this sound @dmfalke?

chowington · 2021-12-07T19:45:03Z

Code looks good, works great.

It would be nice to have the mosaic modal have the table tabs too, but isn't this going to be a nightmare to implement? Instead of passing modalComponentProps to FacetedMosaicPlot we'll have to pass an actual component. Maybe that's an OK hack?

Alternatively, we could have the click-to-expand feature for the contingency tables as well, separate from the faceted mosaics. Would that be useful? I forgot what the contingency tables look like in faceted mode. UPDATE: I don't think the contingency tables need an expanded view, as they're not compressed in the first place.

I am itching to get this merged though, because if another PR gets merged beforehand it could be a nasty merge conflict :)

chowington · 2021-12-08T21:40:55Z

I'm getting ready to wrap this up on the web-components side. Just wanted to get one more review from @dmfalke on this particular commit and I'll merge: ac3926d

dmfalke

Looks good!

dmfalke · 2021-12-08T21:53:10Z

src/plots/FacetedPlot.tsx

+export interface FacetedPlotPropsWithRef<D, P extends PlotProps<D>>
+  extends FacetedPlotProps<D, P> {
+  facetedPlotRef?: Ref<FacetedPlotRef>;
+}


chowington · 2021-12-08T22:01:56Z

Thanks Dave!

Add faceting story to each plot

131e7d1

chowington requested review from bobular and dmfalke November 22, 2021 23:04

chowington added 2 commits November 24, 2021 15:40

Create FacetedBarplot component

22f52fd

Merge branch 'individual-facetedplot-components' into faceting-#259

b73ea05

bobular approved these changes Nov 30, 2021

View reviewed changes

chowington added 4 commits November 30, 2021 18:25

Nonfunctional FacetedBarplot using FacetedPlot

9f8d027

Add basic functionality to expand faceted plot to modal

5d00b2a

Faceted plot modal cleanup

46f6fcd

Uncomment FacetedBarplot which now mysteriously works

2de15e7

chowington added 5 commits December 2, 2021 14:34

Improve faceting modal close button

6b545d0

Better modal plot style in story

d2291de

Change order of props

280d244

Add unique FacetedXPlot component for each plot

74cea42

Fix faceting plot titles bug

672b2cc

chowington marked this pull request as ready for review December 3, 2021 01:27

bobular approved these changes Dec 3, 2021

View reviewed changes

Fix pointer cursor bug

3d092eb

chowington added 2 commits December 6, 2021 17:49

Fix faceted pie plot click bug

3575d2b

Move pie plot onPlotlyRender call to FacetedPiePlot

48483ba

Add comment

282e695

bobular approved these changes Dec 7, 2021

View reviewed changes

dmfalke approved these changes Dec 7, 2021

View reviewed changes

chowington added 7 commits December 7, 2021 16:18

Change a couple of default faceted plot styles

51185dd

Export default faceted plot styles

fc75038

Make FacetedPlotPropsWithRef

ac3926d

Add button title

fa38b76

Increase pixels per character in mosaic plot

b1a7774

Reorder component props

eca0bbc

Update faceted plot default styles

795102b

chowington requested a review from dmfalke December 8, 2021 21:41

dmfalke approved these changes Dec 8, 2021

View reviewed changes

dmfalke reviewed Dec 8, 2021

View reviewed changes

chowington merged commit 2c2496c into main Dec 8, 2021

chowington deleted the faceting-#259 branch December 8, 2021 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faceting #259 #272

Faceting #259 #272

chowington commented Nov 22, 2021

dmfalke commented Nov 23, 2021

bobular left a comment

bobular commented Dec 2, 2021

dmfalke commented Dec 2, 2021

chowington commented Dec 3, 2021

bobular left a comment •

edited

Loading

bobular commented Dec 3, 2021

chowington commented Dec 3, 2021

chowington commented Dec 3, 2021

bobular commented Dec 3, 2021 via email

chowington commented Dec 3, 2021 •

edited

Loading

chowington commented Dec 6, 2021

bobular commented Dec 7, 2021

dmfalke commented Dec 7, 2021

bobular left a comment •

edited

Loading

bobular Dec 7, 2021

dmfalke commented Dec 7, 2021

dmfalke left a comment

chowington commented Dec 7, 2021

chowington commented Dec 7, 2021 •

edited

Loading

chowington commented Dec 7, 2021 •

edited

Loading

chowington commented Dec 8, 2021 •

edited

Loading

dmfalke left a comment

dmfalke Dec 8, 2021

chowington commented Dec 8, 2021

Faceting #259 #272

Faceting #259 #272

Conversation

chowington commented Nov 22, 2021

dmfalke commented Nov 23, 2021

bobular left a comment

Choose a reason for hiding this comment

bobular commented Dec 2, 2021

dmfalke commented Dec 2, 2021

chowington commented Dec 3, 2021

bobular left a comment • edited Loading

Choose a reason for hiding this comment

bobular commented Dec 3, 2021

chowington commented Dec 3, 2021

chowington commented Dec 3, 2021

bobular commented Dec 3, 2021 via email

chowington commented Dec 3, 2021 • edited Loading

chowington commented Dec 6, 2021

bobular commented Dec 7, 2021

dmfalke commented Dec 7, 2021

bobular left a comment • edited Loading

Choose a reason for hiding this comment

bobular Dec 7, 2021

Choose a reason for hiding this comment

dmfalke commented Dec 7, 2021

dmfalke left a comment

Choose a reason for hiding this comment

chowington commented Dec 7, 2021

chowington commented Dec 7, 2021 • edited Loading

chowington commented Dec 7, 2021 • edited Loading

chowington commented Dec 8, 2021 • edited Loading

dmfalke left a comment

Choose a reason for hiding this comment

dmfalke Dec 8, 2021

Choose a reason for hiding this comment

chowington commented Dec 8, 2021

bobular left a comment •

edited

Loading

chowington commented Dec 3, 2021 •

edited

Loading

bobular left a comment •

edited

Loading

chowington commented Dec 7, 2021 •

edited

Loading

chowington commented Dec 7, 2021 •

edited

Loading

chowington commented Dec 8, 2021 •

edited

Loading