Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group rows API #823

Open
aleemstreak opened this issue Dec 13, 2023 · 30 comments
Open

Group rows API #823

aleemstreak opened this issue Dec 13, 2023 · 30 comments
Labels
package:core Requests related to the core package type:enhancement Requests for enhancements or new features

Comments

@aleemstreak
Copy link

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):
Screenshot 2023-12-12 at 10 09 01 PM

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.

@jassmith
Copy link
Contributor

jassmith commented Dec 13, 2023

So the implementation of this is fairly straightforward:

  • Make it possible to efficiently not draw vertical borders for certain rows without spanning the columns. This is going to make making your header easier.
  • Add an API for marking a row as sticky
  • When drawing the header texture, it also needs to draw the overflow region
  • Update the input handler to understand these sticky rows when computing the click item.

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.

@aleemstreak
Copy link
Author

aleemstreak commented Dec 13, 2023

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:

output

@aleemstreak
Copy link
Author

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:

summary

@aleemstreak
Copy link
Author

(and def agree on making this a first class feature because of the input handling)

@jassmith
Copy link
Contributor

jassmith commented Dec 15, 2023

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.

@jassmith jassmith added the type:enhancement Requests for enhancements or new features label Dec 15, 2023
@aleemstreak
Copy link
Author

Thanks for sketching that out.

On scope:

  • might be better to first design this as a row grouping API where all the rows below a group row belong to that group row
    • would need to have first class APIs for collapsing/expanding a group, selecting an entire group, scrolling to a group, etc
  • the sticky part is just an optional part of group rows, you can make your group rows sticky or not

On naming:

  • what about Group Rows
  • it lets you still call them rows so you can maintain a flat data structure, but it implies that there are behaviors that can affect other rows

On implementation:

  • for the verticalBorders option, did you see this comment and this one, how do you think we should handle it?

@jassmith
Copy link
Contributor

jassmith commented Dec 15, 2023

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;
}

@jassmith
Copy link
Contributor

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.

@jassmith
Copy link
Contributor

Another random thought: Any time this prop changes at all blit needs to be calculated false.

@aleemstreak
Copy link
Author

Sorry for the delay, we were discussing a bit internally.

  1. How are you thinking about the isCollapsed property? Should the grid really handle that or should the app developer? what if the app developer had a huge dataset and one group that had a lot of data was collapsed, the grid shouldn't request the data thats in those rows?
  2. Small thing but we’ll also need to handle Cmd up/down a little different for sticky rows too, because the cursor should stop at group boundaries
  3. Seems like there is another issue linked here for frozen rows, how do you see these group rows interacting with frozen rows? And how would the API surfaces interact?

@jassmith
Copy link
Contributor

  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.

@aleemstreak
Copy link
Author

  1. Ah line numbers - that makes sense. Yes, we'll have to be careful that the grid never tries to request any data about rows in collapsed sections.
  2. Great!
  3. I think frozen rows behave differently visually than sticky rows though. Like a frozen row would always be there, pinned to the top where as a sticky row gets scrolled/pushed away by the next sticky row.

@jassmith
Copy link
Contributor

jassmith commented Dec 21, 2023

Sorry I wasn't clear. A singular frozen row is visually equal to

rowGroups: {
  groups: [0],
  makeGroupHeadersSticky: true,
  height: 32,
}

@jassmith
Copy link
Contributor

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.

@jeffersonswartz
Copy link

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.

@jassmith
Copy link
Contributor

Sticking them at the bottom is significantly harder because the trailing row affordance is currently not easily expandable.

@aleemstreak
Copy link
Author

Ohh the trailing rows, right. We'll need to do a trailing row for each row group.

@jassmith
Copy link
Contributor

yall are killing me here

@aleemstreak
Copy link
Author

I think we could implement trailing rows per group in user space actually

@jassmith
Copy link
Contributor

jassmith commented Dec 28, 2023

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.

@aleemstreak
Copy link
Author

Ohhh interesting wrt to the subgroups. How were you expecting them to behave visually and sticky wise?

@jassmith
Copy link
Contributor

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.

@aleemstreak
Copy link
Author

Do you have a motivating use case for the sub groups? Trying to see what the minimal thing could be

@jassmith
Copy link
Contributor

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.

@LukasMasuch
Copy link
Collaborator

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?
    image
    image
    image
  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"> {
  ...
}

@jassmith
Copy link
Contributor

  1. Yes this understanding is correct. We do not manage your data structure. This is a fundamental difference between GDG and many other data tables. There are projects out there that do just the data layer management that would pair nicely.
  2. Technically yes but the 2nd example is far and away the hardest to achieve.
  3. I think this is an area were discussion is needed. More below.
  4. Agree, would love to see this hook come to exist.

Row markers

There are 2 "axes" of row markers when it comes to this feature

  • Row markers show on group header rows or not
  • Row marker counts include group headers, skip group headers, or reset for each group

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.

@jassmith
Copy link
Contributor

There also needs to be a callback for expanding a row when a cell in a collapsed row is activated or someone pressed right.

@LukasMasuch LukasMasuch added the package:core Requests related to the core package label Dec 30, 2023
@jassmith
Copy link
Contributor

jassmith commented Feb 5, 2024

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.

@KyDenZ
Copy link

KyDenZ commented Feb 6, 2024

Continuing on AGGgrid, could we consider similar functionality for columns to support pivot mode display?

Screenshot 2024-02-06 at 20 23 40

@NGimbal
Copy link

NGimbal commented Jun 19, 2024

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.

  1. 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.

  2. 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.

  3. 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 🙏

Screen.Recording.2024-06-19.at.9.00.32.AM.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:core Requests related to the core package type:enhancement Requests for enhancements or new features
Projects
None yet
Development

No branches or pull requests

6 participants