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

New component: Grid #583

Closed
6 tasks
olemartinorg opened this issue Oct 24, 2022 · 12 comments · Fixed by #1118
Closed
6 tasks

New component: Grid #583

olemartinorg opened this issue Oct 24, 2022 · 12 comments · Fixed by #1118
Assignees
Labels
area/table related to grid/list/repeating groups feature-complete Features needed for parity with Altinn 2 (target: June 2023) kind/feature-request New feature or request org/krt org/ssb Issues relevant for Statistisk sentralbyrå.

Comments

@olemartinorg
Copy link
Contributor

olemartinorg commented Oct 24, 2022

Description

After a refinement meeting for #107, we found that some specific of the needs can be isolated into a separate (new) studio/app-frontend layout component: Grid (see my comment in #543).

Example:

20221024_13h34m08s_grim

Functional requirements

Additional Information

@olemartinorg olemartinorg added kind/feature-request New feature or request area/table related to grid/list/repeating groups labels Oct 24, 2022
@olemartinorg olemartinorg moved this to 📈 Todo in Team Apps Oct 24, 2022
@olemartinorg olemartinorg removed the status in Team Apps Oct 24, 2022
@rvessb
Copy link

rvessb commented Oct 27, 2022

org/ssb

@olemartinorg olemartinorg added the org/ssb Issues relevant for Statistisk sentralbyrå. label Oct 27, 2022
@ivarne
Copy link
Member

ivarne commented Oct 27, 2022

org/krt (finanstilsynet)

@olemartinorg
Copy link
Contributor Author

We discussed this a bit during the sprint planning earlier today. Some questions came up. It seems like this task is ready for someone to start working on it, but some specifics are not entirely decided on yet.

So, regarding how to define this in the layout JSON files: Should we support multiple components inside a single cell? Maybe not now, but it might be needed later. In that case, we'll need designs for how that should look (and how two input fields should be differentiated). I suggest that we only support one field per cell for now (and only simple components in them, so no support for the AddressComponent, for example).

The other question was regarding how to reference the components that should live inside the cells. The way I see it, we have two alternatives; we either keep on doing the same thing as it's been done in (repeating) groups, by referencing children externally:

[
  {
    "id": "myGroup",
    "type": "Group",
    "children": ["myComponent1", "myComponent2"]
  },
  {
    "id": "myComponent1",
    "type": "Input"
  },
  {
    "id": "myComponent2",
    "type": "Input"
  },
]

But for a Grid, we'll need a two-dimensional way of referencing children:

{
  "id": "myGrid",
  "type": "Grid",
  "rows": [
    ["myFirstRowComponent1", "myFirstRowComponent2"],
    ["mySecondRowComponent1", "mySecondRowComponent2"]
  ]
}

When thinking more about it:

  1. We'd probably need some way to indicate that a cell (or an entire row/column?) should contain headers, or if a row should be readOnly (i.e. be yellow).
  2. It'd be nice not having to reference a Header or Paragraph component in order to just display some text (so, have a shortcut for that).
  3. It could also be nice not having to reference a child, but defining it directly in the configuration.

All in all, we should take care to find a configuration format that everyone can live with (and still supports our other functionality, like dynamics/expressions, calculation, validation, etc) without too much hassle. We should probably also talk to Team Studio when deciding on a format for this configuration, so we find something they're happy with implementing in Studio.

@ivarne
Copy link
Member

ivarne commented Jan 3, 2023

  • Inlining instead of referencing children would be great (but then we would need to change Group to accept direct children as well for consistensy).
  • Another option to avoid nested lists, would be to have a required property numColumns, so that you can just have a list of children, and break it into the desired number of columns on display.
{
  "id": "myGrid",
  "type": "Grid",
  "numColumns": 2
  "rows": [
    "myFirstRowComponent1",
    "myFirstRowComponent2",
    "mySecondRowComponent1",
    "mySecondRowComponent2"
  ]
}

@olemartinorg
Copy link
Contributor Author

  • Inlining in Group is harder, as that would require support in Studio as well. We can get away with doing pretty much whatever we want here, as the Grid component doesn't have support in Studio yet. Also, we'd have to find a different way to support multiPage if inlining components in Group. Even though I'd love to find a better solution for that mess, I think inlining Group would have to be a separate task.
  • Thanks for the numColumns alternative! I personally find that I much prefer other representations, but I guess this is a contender as well.
  • Yet another alternative is to have each row be an object (with relevant options like header and readOnly), and then have each component reference as an object as well (which can also be other types, like lightweight text references instead of full-blown components):
{
  "id": "myGrid",
  "type": "Grid",
  "rows": [
    {
      "header": true,
      "cells": [
        {"text": "My header text"},
        {"component": "myFirstRowComponent1"}
      ]
    },
    { "cells": [
      {"text": "my.text.key"},
      {"component": "mySecondRowComponent1"}
    ]},
    {
      "readOnly": true,
      "cells": [
        {"text": "texts.sum"},
        {"text": ["sum",
          ["component", "myFirstRowComponent1"],
          ["component", "mySecondRowComponent1"]
        ]}
      ]
    }
  ]
}

@FinnurO
Copy link

FinnurO commented Jan 5, 2023

Eksempel fra SSB
image

@ivarne
Copy link
Member

ivarne commented Jan 5, 2023

We probably need some way to support colspan and rowspan also. That might talk in favor of a more verbose grid layout than a simple list and numCols

@nkylstad nkylstad moved this to 📈 Todo in Team Studio Feb 27, 2023
@nkylstad nkylstad moved this from 📈 Todo to 👷 In Progress in Team Studio Feb 27, 2023
@nkylstad
Copy link
Member

From a studio perspective, the formats discussed here seem like ok alternatives to implement. If there is a need for a lot of configuration on the row level, then the verbose format where each row is an object seems like the best solution.

Restricting the components that can be used inside a table cell to basic form components seems like a good idea, we want to avoid things like nested tables 😅

Do we need settings on a row level, or should these be part of the cell object? Or both?

@nkylstad nkylstad moved this from 👷 In Progress to 📈 Todo in Team Studio Feb 27, 2023
@olemartinorg
Copy link
Contributor Author

@nkylstad Thanks for feedback! 🥳 I'm currently working on the assumption that we'll allow some row-level configuration AND cell-level configuration. That means, you could set rows[0].cells[0].readOnly = true if you'd like, or the whole row by setting rows[0].readOnly = true (which would override the setting for every cell in that row).

For now, we have defined the following typescript types:

export interface GridCellOptions {
  header?: boolean;
  readOnly?: boolean;
}

export interface GridComponentRef extends GridCellOptions {
  component: string;
}

export interface GridText extends GridCellOptions {
  text: ExprVal.String;
}

export type GridCell<C = GridComponentRef> = C | GridText | null;

export interface GridRow<C = GridComponentRef> extends GridCellOptions {
  cells: GridCell<C>[];
}

export interface ILayoutCompGrid<C = GridComponentRef> extends ILayoutCompBase<'Grid'> {
  rows: GridRow<C>[];
}

So, every option in GridCellOptions is also possible to set on GridRow.

@TomasEng
Copy link

TomasEng commented Feb 28, 2023

I am fine with most of the suggestions here. If the components are not going to be referenced outside of the table, I am in favor of inlining the cell configuration directly. Has there been any discussion on how to support summing of cells? Should we support other aggregate functions (average etc.) as well? I don't find the array syntax in the example quite intuitive. Could we do something like this instead?

{
  "cells": [
    {
      "function": {
        "name": "sum",
        "args": [ ... ]
      }
    }
  ]
}

@olemartinorg
Copy link
Contributor Author

@TomasEng We're not aiming for inlining of components in the first implementation. We might go there later (then also for groups), but right now most legacy code in app-frontend would not support it, so we'd have to do some major cleanup first.

Also, sum/average is not really functionality we're supporting in this component, as it's already implemented in single fields by use of calculation (either backend or frontend). We have no explicit support for neither sum or average there, as the exact logic is something the application developers need to decide upon, and implement.

The syntax you describe above looks a lot like what we discussed when developing dynamic expressions, although we arrived at a bit simpler where the expression is an array where the function name comes first and is followed by arguments, so ["sum", ...]. Currently, support for calculation using dynamic expressions is described in #646, but will be separated out to its own issue when time comes for that.

@TomasEng
Copy link

@olemartinorg, okay, so functions are out of scope of this task, I see. Then I have no more objections 🙂

@olemartinorg olemartinorg moved this from 👷 In Progress to ⚠️ Blocked in Team Apps Mar 3, 2023
@HanneLauritsen1967 HanneLauritsen1967 moved this from ⚠️ Blocked to 👷 In Progress in Team Apps Mar 10, 2023
@nkylstad nkylstad moved this from 📈 Todo to ✅ Done in Team Studio Mar 10, 2023
@olemartinorg olemartinorg moved this from 👷 In Progress to 🔎 Review in Team Apps Mar 30, 2023
@olemartinorg olemartinorg moved this from 🔎 Review to 👷 In Progress in Team Apps Mar 30, 2023
@olemartinorg olemartinorg moved this from 👷 In Progress to 📈 Todo in Team Apps Mar 30, 2023
@RonnyB71 RonnyB71 moved this from 📈 Todo to 👷 In Progress in Team Apps Mar 31, 2023
@olemartinorg olemartinorg moved this from 👷 In Progress to ⚠️ Blocked in Team Apps Apr 19, 2023
@olemartinorg olemartinorg moved this from ⚠️ Blocked to 🔎 Review in Team Apps Apr 25, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Issues SSB Apr 26, 2023
@github-project-automation github-project-automation bot moved this to Ready for dev in Design Altinn 3 Apr 26, 2023
@olemartinorg olemartinorg moved this from 🔎 Review to 🧪 Test in Team Apps Apr 26, 2023
@lassopicasso lassopicasso moved this from 🧪 Test to ✅ Done in Team Apps May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/table related to grid/list/repeating groups feature-complete Features needed for parity with Altinn 2 (target: June 2023) kind/feature-request New feature or request org/krt org/ssb Issues relevant for Statistisk sentralbyrå.
Projects
Archived in project
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants