allow collapsing the data section

[keewis]

• [x] Closes #145
• [ ] Tests added
• [x] Passes pre-commit run --all-files
• [ ] Changes are summarized in docs/source/whats-new.rst

In the process I refactored the code to:

• use CSS classes instead of style tags
• match the style of xarray's html formatting code
• only include the CSS stylesheet / SVG icon once per repr

However, I'm not proficient with CSS so I'd appreciate someone looking over this. @benbovy, might I ask you for a review when you have time?

Some new issues:

• ~the attrs of the node data seem to now be connected to the data vars: only attrs disappear when toggled if the data vars are collapsed.~ fixed by putting the sections into <ul> / <li> tags

Some old issues:

1. the vertical line connecting the parent with the children does not always align with the horizontal line of the last node (there's sometimes a 1-2px part that's below the horizontal line... is that browser specific?)
2. the data repr is still small in non-root nodes (#91). Overriding the xr-sections class of the sections with width: 1200px temporarily fixes that issue for me, so I'd assume there's not enough space for the data?

[keewis]

the main goal of allowing to collapse the data attached to a node has been achieved, although this makes #91 worse for me (the workaround of manually setting an arbitrary but sufficient absolute width works, though, so I'd expect this to be a relatively small fix).

The tests are failing because they check equality with a predetermined expected string (which is called acceptance testing, apparently), so they either need to be updated or rewritten to use something similar to xarray's str.count-based tests.

[keewis]

something that just occurred to me is that instead of (or in addition to?) adding a new "data" section we might want to collapse the entire node by clicking on its name. I'd imagine that to be a bit more difficult than the data section, though.

Edit: actually, I see that's what was suggested in #91

Edit: and if we do use the data section we might want to disable it if there's no data? A collapsed Data section would print as Data: (5) otherwise.

[TomNicholas]

Thanks for taking a stab at this @keewis !

the workaround of manually setting an arbitrary but sufficient absolute width works, though, so I'd expect this to be a relatively small fix).

Useful to know.

The tests are failing because they check equality with a predetermined expected string (which is called acceptance testing, apparently), so they either need to be updated or rewritten to use something similar to xarray's str.count-based tests.

Yes I just went with something simple that worked as a test initially.

[benbovy]

@keewis could you upload some screenshots? It will be easier for the review.

A more general comment: why not having a clearer distinction between the data sections and the tree node headers in the repr? Something like https://jsfiddle.net/98k7trvy/. This is a rough example that could be much improved, but it has the advantage of fixing #91 (data sections are less indented than headers, and subtle borders are used to distinguish between data sections).

[keewis]

sure.

The first image is the HTML repr without fixing the width:

while the second has the fixed width (search for style='width: 1200px', I accidentally included that in one of the commits):

[keewis]

why not having a clearer distinction between the data sections and the tree node headers in the repr?

my knowledge of HTML and CSS is pretty limited, so I just used whatever was already available in xarray. If you have a better idea I'd be happy to use that instead!

Edit: oh, but we might want to keep the tree lines at the left

[benbovy]

my knowledge of HTML and CSS is pretty limited, so I just used whatever was already available in xarray.

No worries!

The second screenshot looks already pretty nice. I'd set min-width and max-width instead of setting width for more flexibility, as with a large fixed width there a good chance that the icons on the right won't be accessible on smaller screens or notebook tabs.

Even with the tree lines on the left, I still find that the current repr emphasizes the node data much more than the tree structure (perhaps this could be already improved using a darker gray for the node names), but maybe this is intentional? Also, I don't find Groups vs. Data really useful. I'd rather use the node names as clickable summary headers, although it is probably more work than reuse the HTML / CSS of the Xarray reprs. Anyway, those are general suggestions for further improvement.

Some nits:

• The tree lines could be better aligned with the expand arrows
• The widths of the lines below datatree.DataTree and the node names look a bit weird

[keewis]

the fixed width is a hack, as I had no idea how to fix it otherwise. I'm not sure how exactly min-width works (my guess is that the content should take up anything between min and max), but I have to set it to 800px to get anything remotely readable, which to me indicates that the range does not work as it should.

Since fixing that might involve a fair amount of debugging (even though I expect the fix to be pretty small), maybe it would be better to just reimplement the whole repr using your suggestions? Those do sound good to me, and I had been somewhat discontent with the collapsible Data section, anyways. However, I wonder if we can somehow combine https://jsfiddle.net/98k7trvy/ with the tree lines?

[TomNicholas]

@keewis sorry I forgot about this. Is it still relevant?

[keewis]

yes it is, but I think we're better off redesigning the HTML repr. This is something I wouldn't know too much about (I'm not very familiar with HTML / CSS), but maybe we can make use of anywidget for that?

[benbovy]

FWIW, https://github.com/benbovy/xarray-fancy-repr uses anywidget and implements the Xarray object reprs has a set a reusable react components. I think it would be quite straightforward integrating it with datatree (or once the latter has moved into Xarray) and no need to deal with the hassle of trying to implement some interactivity in pure HTML+CSS. Unfortunately I don't have much time to experiment with this at the moment, though.

[keewis]

I agree, we should put this on hold until the merge is done (and no worries if it ends up taking longer)