Altinn / app-frontend-react

Altinn application React frontend
BSD 3-Clause "New" or "Revised" License
16 stars 24 forks source link

Considerations for v4 #339

Open haakemon opened 1 year ago

haakemon commented 1 year ago

Description

After v3 comes v4. This issues lists changes we need to (or want to) complete in the breaking changes window for v4.

### Breaking changes
- [ ] https://github.com/Altinn/app-frontend-react/pull/1446
- [ ] https://github.com/Altinn/app-frontend-react/pull/1441
- [ ] https://github.com/Altinn/app-frontend-react/pull/1444
- [ ] https://github.com/Altinn/app-frontend-react/issues/1148
- [ ] https://github.com/Altinn/app-frontend-react/issues/975
- [ ] https://github.com/Altinn/app-frontend-react/issues/820
- [ ] https://github.com/Altinn/app-frontend-react/issues/1468
- [ ] https://github.com/Altinn/app-frontend-react/issues/1485
- [ ] https://github.com/Altinn/app-frontend-react/issues/1486
- [ ] https://github.com/Altinn/app-frontend-react/issues/628
- [ ] https://github.com/Altinn/app-frontend-react/issues/1487
- [ ] https://github.com/Altinn/app-frontend-react/issues/1488
- [ ] https://github.com/Altinn/app-frontend-react/issues/1489
- [ ] https://github.com/Altinn/app-frontend-react/issues/1491
- [ ] https://github.com/Altinn/app-frontend-react/issues/1493
- [ ] https://github.com/Altinn/app-frontend-react/issues/1495
- [ ] https://github.com/Altinn/app-frontend-react/issues/1496
- [ ] https://github.com/Altinn/app-frontend-react/issues/1500
- [ ] https://github.com/Altinn/app-frontend-react/issues/1502
- [ ] https://github.com/Altinn/app-frontend-react/issues/1512
### Data model related changes
- [ ] https://github.com/Altinn/app-frontend-react/issues/1175
- [ ] https://github.com/Altinn/app-frontend-react/issues/555
- [ ] https://github.com/Altinn/app-frontend-react/issues/1484
- [ ] https://github.com/Altinn/app-frontend-react/issues/273
- [ ] https://github.com/Altinn/app-frontend-react/issues/925
- [ ] https://github.com/Altinn/app-frontend-react/issues/1111
- [ ] https://github.com/Altinn/app-frontend-react/issues/1147
- [ ] https://github.com/Altinn/app-frontend-react/issues/707
- [ ] https://github.com/Altinn/app-frontend-react/issues/743
- [ ] https://github.com/Altinn/app-frontend-react/issues/1104
- [ ] https://github.com/Altinn/app-frontend-react/issues/1490
- [ ] https://github.com/Altinn/app-frontend-react/issues/1501
### Features, non-breaking and others
- [ ] https://github.com/Altinn/app-frontend-react/issues/754
- [ ] https://github.com/Altinn/app-frontend-react/issues/133
- [ ] https://github.com/Altinn/app-frontend-react/issues/726
- [ ] https://github.com/Altinn/app-frontend-react/issues/1294
- [ ] https://github.com/Altinn/app-frontend-react/issues/1094
olemartinorg commented 1 year ago

I'm also throwing #220 in here. I'm starting to look at bringing this type of functionality (expressions in layout, specifically) into some parts of the frontend in #355 now, but the way we prototyped in #220 meant it would be a breaking change.

olemartinorg commented 1 year ago

Adding a few more issues. Right now it's possible to define a filter for a group component, which can be used to filter out certain rows if they match a value in the data model. It is also possible to define a start/stop index. Showing/hiding rows should use proper dynamics instead, preferably by extending expressions.

bjosttveit commented 1 year ago

Filters on repeating groups appear to be somewhat broken, they currently do not function the way the documentation specifies.

  1. If using more than one filter, only the last filter is actually applied. The documentation says that each element needs to satisfy all filters to be included.
  2. If no elements satisfy the filters, all elements are still included.
  3. The documentation states that "If there is only one result, this is displayed automatically in editing-mode". This does not happen.

The behavior related to adding new elements is also kind of wonky. The add button is still present (unless manually turned off), but if you click it, it will try to add a new element in editing mode which will be immediately filtered out and is not visible at all. Also the add button disappears.

If you edit the field that is filtered on, and change it so that it no longer satisfies the filters, it will be immediately hidden while typing and is no longer possible to access. Maybe it should wait until you click "Save and close" before such a thing happens.

xmrsa commented 1 year ago

Dette forklarer problemene jeg opplevde i mitt skjema, jamfør Slack: https://altinn.slack.com/archives/C02EVE4RU82/p1666346435963569

StianVestli commented 1 year ago

org/ssb

olemartinorg commented 1 year ago

@StianVestli Jeg dropper å tagge dere på dette issuet, siden det bare er en samling av ting vi bør tenke på til neste major-versjon (og dermed er ikke dette noe vi skal "gjøre") - vi kan heller tagge dere på de spesifikke sakene.

olemartinorg commented 1 year ago

Yet another challenge.. Currently we have useDelayedSaveState set up to handle delayed saving of the form (for when you're typing something and we don't want to save that to the server immediately for every keypress). While that's fine for text inputs, parts of it breaks when using it in checkboxes. The problem there is that checking a checkbox and then very quickly checking the next one might cause slowness from the first check to overwrite the second.

This can be reproduced by opening CheckboxesContainerComponent.tsx and setting the delay from 200ms to 800ms, then running the summary.js Cypress test.

We should consider fixing this by introducing a delayed saving saga instead - where all form changes should be batched up so we're not saving every time there is a change/blur. This might require breaking changes around the headers we pass to the backend detailing which component triggered a change - we should look into doing a full diff of the data model instead, and pass that info to the backend.

olemartinorg commented 1 year ago

Now that dynamics can hide entire pages, the PageOrder functionality seems to just create trouble for us, so we should consider removing it (and the calculatePageOrder trigger) to simplify page navigation. Using that in combination with expressions have proven to be problematic.

Read more about this:

StianVestli commented 1 year ago

Skeptisk til at man vil fjerne pageorder funksjonaliteten, siden denne gir oss muligheten til å manipulere data, og gjøre større endringer , endre rekkefølge på sidene osv, det går ikke med ny dynamikk. Da er er det bedre å si at man ikke kan kombinere dynamisk skjuling og pageorder. Eller om man kan manipulere Hidden propertyen via c# koden i pageorder. Fordlene med dynamikken er at den gjenspeiles i summary, det gjør ikke pageorder.

olemartinorg commented 1 year ago

@StianVestli Vi bør ikke ha to måter å gjøre det samme på. Den eneste fordelen jeg har sett med PageOrder er endring av rekkefølge, og potensielt det faktum at man må styre når det trigges (slik at man f.eks. kan skjule siden man står på i det øyeblikket man går til neste). Rekkefølge tenker jeg vi kan styre på andre måter. I dag finnes det opsjoner man kan sette på en layout som styrer hva som skal være neste og forrige side fra der man står, og i fremtiden tenker jeg det bør kunne styres med dynamikk. Uansett, litt av tanken med neste major-versjon er at vi rydder opp i eget hus - og det inkluderer å fjerne kode istedenfor å ha "både og"-funksjonalitet slik som dette.

Viktigst av alt: Om vi ikke får på plass alternativer som dekker alle eksisterende use-case kan vi naturligvis heller ikke fjerne funksjonalitet, nei.

olemartinorg commented 1 year ago

Work has now started, and the changes are currently kept in the next branch.

bjosttveit commented 1 year ago

Another consideration.

I think the "Group" concept should be more general, and that perhaps there should be several components that can have child components. For example, there could be an abstract subclass that all such components implement, that will be used to build the node hierarchy. Today, the Group type is hardcoded as the only such component.

With this in mind, I would like to see the current Group component split into several separate components. Today, the Group component is rendered using three different React components depending on some props:

  1. Repeating group: if maxCount > 1.
  2. PanelGroupContainer: if panel is not null/undefined. Used for adding new elements to another repeating group with a groupReference, or simply as a group with a background color if no groupReference is specified.
  3. DisplayGroupContainer: otherwise. Plain old group with an optional title, makes it easier to create a Summary of a collection of components if fine grained control is not necessary.

To make this work, there is a GroupRendererer component that uses the appropriate react component depending on the props, this logic also needs to be implemented separately in the SummaryComponent. This is not currently implemented for the panel case, and will probably just be rendered the same way as a plain group.

This makes the Group component needlessly complex, and a lot of logic to check the type of Group component is required in many different places to make sure it behaves as expected. This all needs to be maintained during refactoring to make sure no weird use cases break. I think it is a bad idea to keep extending this component and increasing its complexity as opposed to creating several "Grouping" components with a more specific purpose for each.

Therefore, in a new major version, I would like to split the Group component into for example:

This could be implemented in a way that most of the logic reside within the component classes, so that they are responsible for building and maintaining their sub-trees in the hierarchy. Maybe with common interfaces for getting all of their children and performing validations against their children.

olemartinorg commented 1 year ago

Another task we should consider:

There might also be more issues with possible breaking changes regarding design, and I think it would serve us well to go over the current app configuration and think through if we want to do some things differently in v4.

olemartinorg commented 8 months ago

Re: https://github.com/Altinn/app-frontend-react/pull/1449, we should also make sure to detect the backend version and deny running on older/legacy backend versions. I'm hoping we can check for multipart-saving support on the backend for this, if that gets implemented on the backend any time soon - otherwise have have no reliable way of checking.