Improve whitespace for small multiples #575

dlaliberte · 2020-10-17T01:08:57Z

closes #569

Prerequisites:

Unless it is a hotfix it should be merged against the dev branch
Branch is up-to-date with the branch to be merged with, i.e. dev
Build is successful
Code is cleaned up and formatted

Summary

Turn off the autosize mode for the small multiples so we can give them all the same padding.

netlify · 2020-10-17T01:10:30Z

Preview link ready!

Built with commit 8ce7bc3

https://deploy-preview-575--cmu-delphi-covidcast.netlify.app

sgratzl · 2020-10-19T12:16:47Z

can you do me a favor and add screenshots or for an interactive change a screencast such that it is easier to see what has changed?

dlaliberte · 2020-10-19T12:43:50Z

can you do me a favor and add screenshots or for an interactive change a screencast such that it is easier to see what has changed?

Also added to #569:

sgratzl

I'm getting a bunch of warnings in the console now:

WARN Height "container" only works well with autosize "fit" or "fit-y".

can you take a look what is going on?

dlaliberte · 2020-10-19T15:21:22Z

Well, the change depends on disabling autosize: 'fit', the default, so I guess we have to change the height: 'container' to something else. Or maybe is there another way to avoid getting the warnings?

…

On Mon, Oct 19, 2020 at 9:01 AM Samuel Gratzl ***@***.***> wrote: ***@***.**** commented on this pull request. I'm getting a bunch of warnings in the console now: WARN Height "container" only works well with autosize "fit" or "fit-y". can you take a look what is going on? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#575 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAELGDKTZVO7QH7PAWQ6EQTSLQ2ELANCNFSM4ST7WCLQ> .

-- Daniel LaLiberte <https://plus.google.com/100631381223468223275?prsrc=2> dlaliberte@Google.com <dlaliberte@google.com> Cambridge MA

…ng.right, to avoid clipping.

dlaliberte · 2020-10-19T15:54:37Z

I hadn't noticed before, but the right edge of the charts was being cropped. The last commit fixes that as well as avoiding the warnings. It adds more padding between the charts and the scrollbar, but that appears to be outside of the charts.

sgratzl · 2020-10-19T18:37:59Z

src/modes/overview/vegaSpec.js

-    width: 'container',
-    height: 'container',
-    padding: 0,
+    width: '378',


it is difficult to specify a precise number, since you don't know how wide the side panel is in every case, since it depends on the screen resolution. Especially on mobile the size is very different. You could use a similar thing patchSpec as in the detail view, which allow you to specify the width / height based on the container width.

Yes, I didn't like specifying precise numbers, but I just wanted to make it work at first. I'm not sure I like the patchSpec approach either, since it is kind of convoluted, and it requires a different call for each use. It would be better if we could calculate it in a general way from the actual container size, however that is determined. I'll see what I can do.

I'm stuck. I suspect the warning when using 'container' with autosize: 'none' is superfluous, or at least in our case. autosize: 'none' does the right thing regarding padding, and no other mode does that, as far as I can tell. Also, width and height set to 'container' does the right thing regarding determining the correct container size, and nothing else does that. So we need both together.

I don't know if we care about auto sizing based on a change of the container size, if we can handle that another way. If we can do that, then it would make most sense to disable the warning. I don't see any way to do that in the vega-lite code. This 'container' feature is new from about a year ago, so perhaps they haven't worked out all the bugs. I can follow up with the vega-lite people to see what they think.

Catching the log.warn and throwing it away would be one possible workaround.

Alternatively, determining the container width and height ourselves, and passing that in via something like patchSpec could work, but I haven't been able to figure out how to make that happen. The patching has to be done differently from the DetailView case, of course, since the small multiples don't use vconcat, and it doesn't seem to work correctly if I wrap the current spec with a single vconcat so that patch could add another with size constraint. But moreover, the spec itself isn't passed in to the Vega component the same way as for the DetailView, and I can't tell how to make it work given my limited knowledge of svelte. Maybe you could get this working much more quickly than all my thrashing about.

Another alternative to consider is to merge all these small multiples into one chart, using vconcat. That is a substantial change, but I think I could handle the vega part of it.

We could save some vertical space by eliminating all but one of the domain axes, since they are now all the same, and we have the parallel selection across all of the charts.

I added a comment to the vega-lite change that added the log warning. See vega/vega-lite#5441
Dominik replied, supporting my suggestion, and I would guess we should be able get this fixed at some point in vega-lite.

Meanwhile, I am wondering if we could recompute the width and height using a signals. See https://observablehq.com/@dlaliberte/vega-top-level-width-height-padding-autosize-options Looks like it should work at last for Vega, and I believe Vega-Lite works the same. I'll investigate.

Using signals via the vega-embed patch appears to be working, at least partly. I have to add separate signals for both width and height, and separate signals for both resize and load (without the load event, the initial size will be whatever the default value is). When Switching to compare mode, the initial display of the small multiples is the wrong size, and this happens even though I am trying to use the observeResize to dispatch a window resize, in order to trigger the update of the vega chart size(s). I'll futz with this some more to try to get it to behave, but maybe we need some more explicit resize triggers when we know there are events that need to update the charts.

By the way, the map is also not being updated based on the dispatched resizes. Manually resizing the window does produce the desired effect (updating everything) so this is suspicious.

Adding the following in Vega.svelte:

afterUpdate(() => { if (!patchSpec) { window.dispatchEvent(new Event('resize')); } });

I am able to get the small multiples to behave correctly on the Map Overview page, also when in Compare mode, and when resized for mobile devices or during any resizing events, without affecting the DetailView. However, then I learned that the overview/vegaSpec.js is also used by the top 10 and Region Details pages, which rely on different container sizes. Fortunately, by adding the resize signals for the height as well as width, all views appear to work correctly, except for one: When combining the DetailView with compare mode, then the DetailView chart overflows its container. I suspect the resize events are triggering the patchSpec call to detect the wrong width at that time, before some other adjustment takes place.

From reply to email from netlify:

Replacing the dispatch resize event with your suggested vega.view.resize().runAsync() results in a very messed up display: empty map, and it fails after trying to draw the first small multiple.

By the way, the DetailView sizing problem also occurs even without going into "compare mode", as soon as a small multiple tooltip is shown.

I'm not seeing any performance problem with the multiple resize dispatches, so I suspect the resize events are debounced, and maybe vega is detecting that there is no size change. However, I am seeing an afterUpdate call for each chart just due to showing a tooltip in any one of the charts. That explains the DetailView sizing problem showing up after showing a tooltip.

So I am inclined to think that the DetailView could be fixed to avoid this problem. I'd rather avoid doing the patchSpec call at all, if we can properly detect the container size, while also avoiding use of the arbitrary offsets to "correct" the sizing. I'll look into that.

On Mon, Oct 26, 2020 at 8:28 AM Samuel Gratzl notifications@github.com wrote:
@sgratzl commented on this pull request.

In src/components/Vega.svelte:

@@ -209,6 +237,12 @@
}
});

afterUpdate(() => {

if (!patchSpec) {
is there a difference to triggering vega.view.resize().runAsync()? cause in this in case of the small multiples N events are triggered

sgratzl · 2020-10-19T18:38:58Z

src/components/DetailView/vegaSpec.js

@@ -147,6 +145,17 @@ export function createSpec(sensor, primaryValue, selections, initialSelection) {
            },
            encoding: {
              color: colorEncoding(selections),
+              x: {


what is the difference to xDateRangeEncoding ? at a first glance, I cannot spot it

You're right. It evolved over several steps, so I'll make it the same here.

But for the next layer, tickCount is 'day', and the whole axis property is repeated as a consequence. Maye I can just use axis: {...(xDateRangeEncoding.axis), tickCount: 'day'}. I'll try it.

Hang on... this change of Detailview/vegaSpec.js is supposed to be part of a different changeset: #574
Don't know how they got mixed up but maybe I forgot to switch branches. Sigh. Now I have to figure out how to uncommit this part, and/or unpush it, or whatever.

This merge of branch 'dev' into dlaliberte/#563 appears to be the reason why my changes to DetailView/vegaSpec.js are part of this change. How do I undo that?

there is no easy option, the simple option is when it happens that you say in the summary of the PR that it is based on another one. and then we have to make sure that the other one is merged first

Apparently I did this before as well, since you noted in a comment that my change included another one. So I am doing something wrong, but I don't know what I did in order to avoid doing it again. For this change, I am going to start over with another branch, and avoid "synching" until I am forced to.

dlaliberte

This merge of branch 'dev' into dlaliberte/#563 appears to be the reason why my changes to DetailView/vegaSpec.js are part of this change. How do I undo that?

sgratzl · 2020-10-26T12:27:06Z

src/components/Vega.svelte

@@ -209,6 +237,12 @@
    }
  });

+  afterUpdate(() => {
+    if (!patchSpec) {


is there a difference to triggering vega.view.resize().runAsync()? cause in this in case of the small multiples N events are triggered

dlaliberte · 2020-10-26T17:37:58Z

Replacing the dispatch resize event with your suggested vega.view.resize().runAsync() results in a very messed up display: empty map, and it fails after trying to draw the first small multiple. By the way, the DetailView sizing problem also occurs even without going into "compare mode", as soon as a small multiple tooltip is shown. I'm not seeing any performance problem with the multiple resize dispatches, so I suspect the resize events are debounced, and maybe vega is detecting that there is no size change. However, I am seeing an afterUpdate call for each chart just due to showing a tooltip in any one of the charts. That explains the DetailView sizing problem showing up after showing a tooltip. So I am inclined to think that the DetailView could be fixed to avoid this problem. I'd rather avoid doing the patchSpec call at all, if we can properly detect the container size, while also avoiding use of the arbitrary offsets to "correct" the sizing. I'll look into that.

…

On Mon, Oct 26, 2020 at 8:28 AM Samuel Gratzl ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/components/Vega.svelte <#575 (comment)> : > @@ -209,6 +237,12 @@ } }); + afterUpdate(() => { + if (!patchSpec) { is there a difference to triggering vega.view.resize().runAsync()? cause in this in case of the small multiples N events are triggered — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#575 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAELGDPXWX46JM4AYCQT2ZTSMVTO3ANCNFSM4ST7WCLQ> .

-- Daniel LaLiberte <https://plus.google.com/100631381223468223275?prsrc=2> dlaliberte@Google.com <dlaliberte@google.com> Cambridge MA

dlaliberte · 2020-10-29T04:07:20Z

I believe everything is working as it should now, at least for this change. There is severe lag when hovering over the small multiples, however, even though I believe this version should be disabling the 'nearest' mode. Maybe the excess resize events are causing problems, though I wasn't seeing this lag earlier.

But #588 works fast and smooth, so we should merge that in first, and then check out this change again after that.

…569

dlaliberte · 2020-10-30T00:26:18Z

Seems very responsive now. The main trick was to debounce the dispatch of resize events, so that while moving the mouse over a small multiple, drawing tooltips many times, only one set of resize events is dispatched when you stop moving. Occasionally, the small multiple gets initially displayed with the wrong size, but it quickly updates.

src/components/Vega.svelte

sgratzl

👍

Turn off autosize for small multiples, so we can use fixed-size padding.

40ee4e9

dlaliberte requested a review from sgratzl October 17, 2020 01:09

dlaliberte mentioned this pull request Oct 17, 2020

Align left ends of date axis for indicators #569

Closed

sgratzl reviewed Oct 19, 2020

View reviewed changes

Stop using width and height: 'container' to avoid warnings. Add paddi…

1527503

…ng.right, to avoid clipping.

sgratzl reviewed Oct 19, 2020

View reviewed changes

dlaliberte commented Oct 19, 2020

View reviewed changes

tildechris linked an issue Oct 20, 2020 that may be closed by this pull request

Align left ends of date axis for indicators #569

Closed

Use signals to update width/height on load/resize

abbf672

sgratzl reviewed Oct 26, 2020

View reviewed changes

dlaliberte added 5 commits October 26, 2020 19:20

Use patchSpec only to set height.

8c0de12

Tweak height offset

d47efc8

Merge branch 'dev' into dlaliberte/#569

b27c581

More tweaks after syncing with dev updates

0cc8434

Always resize after update.

2993d92

dlaliberte added 6 commits October 29, 2020 15:59

Merge remote-tracking branch 'origin/dev' into dlaliberte/#569

6e24bb7

Merge remote-tracking branch 'origin/dev' into dlaliberte/#569

ed32ee2

Merge remote-tracking branch 'origin/dev' into dlaliberte/#569

c6030cd

Merge remote-tracking branch 'origin/dlaliberte/#569' into dlaliberte/#…

9d0bc53

…569

debounce resize events

e0e4a60

remove console.info

b4c5c0b

sgratzl reviewed Oct 30, 2020

View reviewed changes

src/components/Vega.svelte Outdated Show resolved Hide resolved

src/components/Vega.svelte Show resolved Hide resolved

Remove extraneous condition.

8ce7bc3

sgratzl approved these changes Oct 30, 2020

View reviewed changes

dlaliberte merged commit 7c3f9b6 into dev Oct 30, 2020

dlaliberte deleted the dlaliberte/#569 branch October 30, 2020 21:37

dlaliberte mentioned this pull request Nov 4, 2020

Top 10 list refreshes multiple times, occasionally resulting in incorrect clipping #615

Closed

tildechris changed the title ~~Dlaliberte/#569~~ Improve whitespace for small multiples Nov 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve whitespace for small multiples #575

Improve whitespace for small multiples #575

dlaliberte commented Oct 17, 2020 •

edited

Loading

netlify bot commented Oct 17, 2020 •

edited

Loading

sgratzl commented Oct 19, 2020

dlaliberte commented Oct 19, 2020 •

edited

Loading

sgratzl left a comment

dlaliberte commented Oct 19, 2020 via email

dlaliberte commented Oct 19, 2020

sgratzl Oct 19, 2020

dlaliberte Oct 19, 2020

dlaliberte Oct 20, 2020

dlaliberte Oct 20, 2020

dlaliberte Oct 23, 2020

dlaliberte Oct 23, 2020

dlaliberte Oct 24, 2020 •

edited

Loading

dlaliberte Oct 26, 2020

sgratzl Oct 19, 2020

dlaliberte Oct 19, 2020

dlaliberte Oct 19, 2020

sgratzl Oct 19, 2020

dlaliberte Oct 19, 2020

dlaliberte left a comment

sgratzl Oct 26, 2020

dlaliberte commented Oct 26, 2020 via email

dlaliberte commented Oct 29, 2020 •

edited

Loading

dlaliberte commented Oct 30, 2020

sgratzl left a comment

Improve whitespace for small multiples #575

Improve whitespace for small multiples #575

Conversation

dlaliberte commented Oct 17, 2020 • edited Loading

Summary

netlify bot commented Oct 17, 2020 • edited Loading

sgratzl commented Oct 19, 2020

dlaliberte commented Oct 19, 2020 • edited Loading

sgratzl left a comment

Choose a reason for hiding this comment

dlaliberte commented Oct 19, 2020 via email

dlaliberte commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlaliberte Oct 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlaliberte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlaliberte commented Oct 26, 2020 via email

dlaliberte commented Oct 29, 2020 • edited Loading

dlaliberte commented Oct 30, 2020

sgratzl left a comment

Choose a reason for hiding this comment

dlaliberte commented Oct 17, 2020 •

edited

Loading

netlify bot commented Oct 17, 2020 •

edited

Loading

dlaliberte commented Oct 19, 2020 •

edited

Loading

dlaliberte Oct 24, 2020 •

edited

Loading

dlaliberte commented Oct 29, 2020 •

edited

Loading