xarray-contrib / datatree

WIP implementation of a tree-like hierarchical data structure for xarray.
https://xarray-datatree.readthedocs.io
Apache License 2.0
161 stars 43 forks source link

Rich display width is broken #335

Open eschalkargans opened 1 month ago

eschalkargans commented 1 month ago

Bug description

Screenshot from 2024-05-16 17-58-24

The group width is shrinked, not using the full available width. The width should only be restrained as we progress in the hierarchy, making high level groups almost the same size as classical datasets

We can see more clearly the width issue in this high contrast screenshot:

Screenshot from 2024-05-16 18-03-44

Other bugs

Note: the alternating colors for the data variables rows seem not to take into account the theme. They should be dependent on the them, like the rest of the rich display

etienneschalk commented 1 month ago

Technical analysis

Quick fix

The quick and dirty fix can be the following

-  "<div class='xr-wrap' style='display:none'>"
+ "<div class='xr-wrap' style='display:none; min-width: 700px;'>"

It forces override the min-width: 300px coming from the xr-wrap CSS class, and replace it with the same value as its max-width of 700px.

See the method _obj_repr https://github.com/pydata/xarray/blob/12123be8608f3a90c6a6d8a1cdc4845126492364/xarray/core/formatting_html.py#L297C22-L297C24

See the xr-wrap class https://github.com/pydata/xarray/blob/12123be8608f3a90c6a6d8a1cdc4845126492364/xarray/static/css/style.css#L29

Better fix

The good fix would be to update the class xr-wrap ; however this class is from the xarray's core, so changing it would not only change datatree but xarray too. So, an alternative could be for instance, adding a new class, xr-datatree-wrap, with more-suited width parameters, copied from xr-wrap with a minor change:

.xr-wrap {
  display: block !important;
  min-width: 300px;
  max-width: 700px;
}

.xr-dataset-wrap {
  display: block !important;
  min-width: 700px;
  max-width: 700px;
}

Improvement

The following applies to both xarray and datatree.

The current collapsible section is based on using hidden checkboxes with the CSS :checked selector to reveal or hide the elements. This code is from 5 years ago. This StackOverflow thread shows the current solution being using <details>, and the old solution of using hidden checkboxes and CSS :checked selection as an older solution: https://stackoverflow.com/questions/41220717/collapse-without-javascript

Recently, a new couple of HTML elements have been added, <details> and <summary>, that are the official HTML tags to implemented collapsible sections with little effort. See MDN docs: <details>: The Details disclosure element (widely available on browsers according to this link)

Example (it works with GitHub's markdown)

Example taken from Tree views in CSS

The idea would be to just create a tree structure of nested <section> and <summary>, and delegating the final HTML rendering to the current Dataset code. This can be done for each node, by using to_dataset(), whether the group is a leaf or not. Each node would be: its Dataset (the leaf variables it contains) + its children.

An other alternative, at the cost of a dependency, would be to use ipytree, a Jupyter widget representing a tree. This is a Jupyter widget ; can it be rendered as HTML for export?

keewis commented 1 month ago

This should be a duplicate of #91 (unless I'm misunderstanding something?). Can you try to reproduce with xarray.core.datatree.DataTree? I believe this should have been fixed in pydata/xarray#8930.

etienneschalk commented 1 month ago

Hello, indeed it is a duplicate of #91 and is fixed when using xarray.core.datatree.DataTree!

The point of using <details> and <summary> remains, but as long as the current solution works... I am fine with it!

Is it planned to backport the bugfixes of xarray to this datatree repo, or not (so users are encouraged to use the latest xarray versions)?

TomNicholas commented 1 month ago

Is it planned to backport the bugfixes of xarray to this datatree repo, or not (so users are encouraged to use the latest xarray versions)?

No - this repo will be archived in favour of using xarray itself.