Open aleemstreak opened 11 months ago
So the implementation of this is fairly straightforward:
I think we probably want to make this more of a first class feature and a feature you can build from a collection of features. The largest reason for this is the weird impact it has on input handling and sizing the header texture.
I will flesh out the details soon.
I'm not sure making it a regular row that hides the vertical seperators is going to work because we wouldn't want clipping in the row. For example, imagine the title of the group was really long:
On the other hand, we'll prob need a way to figure out where the columns start and end in row because we might want to draw things in the group row that align with the headers like:
(and def agree on making this a first class feature because of the input handling)
API sketching:
interface StickyRowData {
/**
* The indexes of the rows to make sticky, this must include the 0th row.
* @group Data
*/
readonly rows: CompactSelection;
/**
* The height of the sticky rows. If not provided all rows will get the same height as the first sticky row. All
* sticky rows must have the same height.
*/
readonly height?: number;
/**
* Enables or disables the drawing of borders inside of sticky rows
* @defaultValue true
*/
readonly verticalBorders?: boolean;
/**
* Overrides the default theme of the sticky row
*/
readonly themeOverride?: Partial<Theme>;
/**
* Controls the navigation behavior of the grid. If `skip` is provided the grid will skip over the sticky rows
* when the user selects a new cell. If `skip-up` or `skip-down` is provided the grid will skip over the sticky
* rows when the user navigates up or down.
*
* If a sticky row is marked block, it will act like skip, but clicking on it will also not result in selection
* a cell in the sticky row.
*/
readonly navigationBehavior?: "normal" | "skip" | "skip-up" | "skip-down" | "block";
/**
* Controls the selection behavior of the grid. If spanning is allowed sticky rows act like any other cells. If
* spanning is not allowed selections will be unable to span sticky rows.
*/
readonly selectionBehavior?: "allow-spanning" | "block-spanning";
}
So when creating a DataEditor you would pass a StickyDataRow object if you want to use sticky data rows.
This interface should cover all of your use cases. You can enable and disable the borders inside of sticky rows. You can change the navigation behavior, and you can either allow selections to span over sticky rows or block it. What you cannot do is allow selection to span a sticky row and omit the row itself. This would be possible by leveraging the range stack, but it would be complicated and I don't think it is needed right now.
Implementation is fairly straightfoward. If sticky rows are enabled the header texture target is extended to the height of the sticky rows and we simply compute which 1 or 2 sticky rows need to be rendered into it based on the scroll state. Hit detection needs a similar update, fortunately the same code will do both. The rendering into the header texture is the reason we are restricting that the first row must be a sticky row. Otherwise it makes this slightly more complicated.
Sticky rows will incur a barely perceptible performance penalty during the push part of the scroll. This will not be noticeable.
What do you think?
Some outstanding issues in my mind:
1) I don't like the name sticky rows. I think it... sucks? Yeah it sucks.
2) It seems like this should combine well with some form of grouping behavior. I wonder if there is a reasonable way to express "collapsed rows" as well. This would make this feature not only provide sticky rows like you want but also row grouping.
Thanks for sketching that out.
Group Rows
verticalBorders
option, did you see this comment and this one, how do you think we should handle it?Yeah I saw them, I just decided if you want that you will need to add span cells and deal with that problem yourself. Looking at your interface this is a pretty edge casey edge case :P
Seriously though there is no downside to just letting you specify that cell as a span, or some subset of it. Getting the size of the columns is the only maybe hard part we can work out later.
Updated proposal:
interface RowGroupingOptions {
/**
* The indexes of the rows of the grid which are group headers and their collapse state. If a number is provided
* instead of an object, the group header will not be collapsed.
*/
readonly groups: readonly ({
readonly headerIndex: number;
readonly isCollapsed: boolean;
} | number)[];
/**
* Causes the group headers to collect at the top of the grid. Each replacing the last.
*/
readonly makeGroupHeadersSticky?: boolean;
/**
* The height of the group headers. All group headers must have the same height.
*/
readonly height: number;
/**
* Enables or disables the drawing of borders inside of group headers.
* @defaultValue true
*/
readonly verticalBorders?: boolean;
/**
* Overrides the default theme of the group headers.
*/
readonly themeOverride?: Partial<Theme>;
/**
* Controls the navigation behavior of the grid. If `skip` is provided the grid will skip over the group headers
* when the user selects a new cell. If `skip-up` or `skip-down` is provided the grid will skip over the group
* headers when the user navigates up or down.
*
* If a group header is marked block, it will act like skip, but clicking on it will also not result in selection
* a cell when clicked.
*/
readonly navigationBehavior?: "normal" | "skip" | "skip-up" | "skip-down" | "block";
/**
* Controls the selection behavior of the grid. If spanning is allowed group headers act like any other cells. If
* spanning is not allowed selections will be unable to span group headers.
*/
readonly selectionBehavior?: "allow-spanning" | "block-spanning";
}
interface DataEditorProps {
// ...other props
rowGrouping?: RowGroupingOptions;
}
Since we can easily compute the size of each group it is easy to integrate into the walkRows algorithm to automatically skip the entire group without incurring significant overhead.
The only downside of this is that people trying to make tree structures will have to create really inefficient layouts.
Another random thought: Any time this prop changes at all blit needs to be calculated false.
Sorry for the delay, we were discussing a bit internally.
1) The reason the grid needs to handle the collapsing and not the developer is line numbers will be off otherwise. The grid wont know about the missing rows. The grid wont request that data (unless forced to by a copy operation). It will simply skip the rows when rendering.
2) Yup, easy-peasy.
3) A single frozen row is a special case of this where there is just a single group that is sticky. Multiple frozen rows is a little more complex but I believe for the sake of total compatibility frozen rows might just be 1 or 0.
Sorry I wasn't clear. A singular frozen row is visually equal to
rowGroups: {
groups: [0],
makeGroupHeadersSticky: true,
height: 32,
}
Another thing to remember, the top drop shadow will have to be taught how to play nice with this. I think the simplest move is to fade the shadow out as the new sticky header comes in and the fade it back in once the old one is in place. Other options would be to clip the shadow to not overlay the incoming row. Not sure how to do that tho.
Hi @jassmith, in sticky rows it would be better if we have an ability to stick rows to top and bottom as well, like how we have trailing rows now.
Sticking them at the bottom is significantly harder because the trailing row affordance is currently not easily expandable.
Ohh the trailing rows, right. We'll need to do a trailing row for each row group.
yall are killing me here
I think we could implement trailing rows per group in user space actually
After more consideration I think the API should look like this:
type RowGroup = {
readonly headerIndex: number;
readonly isCollapsed: boolean;
readonly subGroups?: readonly RowGroup[];
} | number;
interface RowGroupingOptions {
/**
* The indexes of the rows of the grid which are group headers and their collapse state. If a number is provided
* instead of an object, the group header will not be collapsed.
*/
readonly groups: readonly RowGroup[];
/**
* Causes the group headers to collect at the top of the grid. Each replacing the last.
*/
readonly makeGroupHeadersSticky?: boolean;
/**
* The height of the group headers. All group headers must have the same height.
*/
readonly height: number;
/**
* Enables or disables the drawing of borders inside of group headers.
* @defaultValue true
*/
readonly verticalBorders?: boolean;
/**
* Overrides the default theme of the group headers.
*/
readonly themeOverride?: Partial<Theme>;
/**
* Controls the navigation behavior of the grid. If `skip` is provided the grid will skip over the group headers
* when the user selects a new cell. If `skip-up` or `skip-down` is provided the grid will skip over the group
* headers when the user navigates up or down.
*
* If a group header is marked block, it will act like skip, but clicking on it will also not result in selection
* a cell when clicked.
*/
readonly navigationBehavior?: "normal" | "skip" | "skip-up" | "skip-down" | "block";
/**
* Controls the selection behavior of the grid. If spanning is allowed group headers act like any other cells. If
* spanning is not allowed selections will be unable to span group headers.
*/
readonly selectionBehavior?: "allow-spanning" | "block-spanning";
}
The primary change here is to allow subgrouping. These groups can then be flattened into a more useful format as so:
type ExpandedRowGroup = {
readonly headerIndex: number;
readonly isCollapsed: boolean;
readonly rows: number;
};
const flattenedRowGroups: ExpandedRowGroup[] = React.useMemo(() => {
if (rowGrouping === undefined) return [];
const flattened: ExpandedRowGroup[] = [];
function processGroup(group: RowGroup, nextHeaderIndex: number | null): void {
const rowsInGroup =
nextHeaderIndex !== null ? nextHeaderIndex - group.headerIndex : rows - group.headerIndex;
flattened.push({
headerIndex: group.headerIndex,
isCollapsed: group.isCollapsed,
rows: rowsInGroup,
});
if (!group.isCollapsed && group.subGroups) {
for (let i = 0; i < group.subGroups.length; i++) {
const nextSubHeaderIndex =
i < group.subGroups.length - 1 ? group.subGroups[i + 1].headerIndex : nextHeaderIndex;
processGroup(group.subGroups[i], nextSubHeaderIndex);
}
}
}
for (let i = 0; i < rowGrouping.groups.length; i++) {
const nextHeaderIndex = i < rowGrouping.groups.length - 1 ? rowGrouping.groups[i + 1].headerIndex : null;
processGroup(rowGrouping.groups[i], nextHeaderIndex);
}
return flattened;
}, [rowGrouping, rows]);
This resulting structure makes it easy to simply skip all rows which are collapsed when walking rows.
Ohhh interesting wrt to the subgroups. How were you expecting them to behave visually and sticky wise?
Great question, I am not intending to allow arbitrary amounts of collection. Such a thing presents significant additional complexity I do not wish to resolve. The only reasonable path forward to making that work is to either create a more complex clipping and blitting algorithm (ew) or to convert the overlay canvas to a full height ARGB canvas. Both of these options are unappealing.
I think either subgroups and sticky are mutually exclusive or only top level groups stick or last header past wins regardless of hierarchy.
Do you have a motivating use case for the sub groups? Trying to see what the minimal thing could be
There is a major 5 letter company using GDG internally that forked it to add the grouping and subgrouping feature. I want to bring them back into the fold as well.
Row grouping is also a feature we would love to use in Streamlit. Our requirements are pretty aligned with the basic examples from aggrid or MUI. But I think subgroups would be a requirement for our use case.
A couple of thoughts and questions on the proposed API:
1) The row group does not automatically add some kind of expander element, or? To my understanding, this would require using something like an expandable tree cell, which is used to set the isCollapsed
property of a row group.
2) From my understanding of the proposed API, all three of the aggrid row grouping display types would be possible in GDG as well, or?
<img width="691" alt="image" src="https://github.com/glideapps/glide-data-grid/assets/2852129/1bf63ec7-ab57-42f5-852e-d7ed3e1a50e6">
<img width="691" alt="image" src="https://github.com/glideapps/glide-data-grid/assets/2852129/e6d291f7-2460-46f5-bde7-e29c3302e855">
3) How would the selection via row markers work for group rows? Would it automatically select all underlying subgroups & rows?
4) The proposed API is quite low-level as most of the other features of GDG. It could be beneficial for this to provide a more high-level hook in the source
package as well, which performs an in-browser group on selected columns and auto-creates the row grouping options. E.g.:
function useRowGrouping(
groupBy: GridColumn[],
numRows: number,
columns: GridColumn[],
getCellContent: ([col, row]: readonly [number, number]) => GridCell
): Pick<DataEditorProps, "getCellContent", "rows", "rowGrouping"> {
...
}
There are 2 "axes" of row markers when it comes to this feature
In my opinion these are separate concerns. It is probably time to consolidate the rowMarkers
API's into a single behavior object which can include all of these options. I don't think there is a right choice for either of these axes, it is situational.
There also needs to be a callback for expanding a row when a cell in a collapsed row is activated or someone pressed right.
Active development here: https://github.com/glideapps/glide-data-grid/tree/jason/row-grouping
Right now the basic feature set works. There are lots of odds and ends to tie up.
Continuing on AGGgrid, could we consider similar functionality for columns to support pivot mode display?
I would like to report a little feedback for the current implementation, I have version "@glideapps/glide-data-grid": "^6.0.4-alpha8"
installed.
First of all thank you for this work! I'm super excited to be able to group by rows with glide data grid.
It seems like row group header height is not taken into account when calculating height for scroll in the data grid? If I set the height to 32px
then scroll works fine, but any higher and I start to lose a few pixels on the bottom.
This is a UX / stylistic comment, and it may be related to the comment above, but would it be possible when collapsing groups for the content to collapse "up" instead of "down"? I think this is a function of the scroll position when grouping? See the attached screen recording for more context.
The group headers seem to "eat a row" in the data table, for instance in the grouped row storybook story, there are only 995 rows + 5 groups, but there should be 1000 rows of data. I worked around this by grouping my data, then adding an additional row for each group while flattening the grouped data. So good so far, but I'm running into a really weird feature in which I need to duplicate the last (not the first? 👀) item in each group to make sure all the rows render. Furthermore I haven't figured out how to insert a grouped row properly such that I can read summary data from that grouped row in my getCellContentMangled
function (probably going to rename this getCellContentGrouped
or something). I think I'm not understanding how the path mapper works, but the general weirdness makes me think there could be a bug at play.
That was my biggest pain point, I'm not sure if you have any thoughts or guidance. Thanks again for your hard work 🙏
https://github.com/glideapps/glide-data-grid/assets/34066664/a1d1c80a-dcbe-400f-84d2-3b3be11d06af
Hi there!
We're from Streak (yc s11) and we're looking to move to Glide Data Grid. One missing feature from our current in-house implementation is the ability to have group rows.
Here's roughly what we mean by a group row (notice all the features are listed in the screenshot):
Here's a video of how the group row interactions feel wrt to scrolling and selection: https://www.loom.com/share/6cdaabf0bc3f402c90099749f2d65ada
Would love to start the discussion on how this might be implemented in GDG, we'd be happy to implement.