Make FormData type stable, and add/improve lots of related Typescirpt typings #7718

ivarne · 2021-12-21T09:30:53Z

When working with Altinn/app-frontend-react#789 I had some problems working around the issue that the existence of dataModelBindings.simpleBinding changed the type of formData to string and prevents me from reading the other dataModelBindings. This pull request fixes that issue, so that formData is always an object that usually has a single property simpleBinding.

Description

In fixing this, I also had to fix lots of typescript types and renamed a function isSimpleComponent -> componentRendersItsOwnValidationErrors. I have tried to group related changes into separate commits, so most likely a reviewer should consider reviewing each commit separately (ps: I know i screwed up so intermediate commits does not all compile).

Fixes

n/a

Verification

Your code builds clean without any errors or warnings
Manual testing done (required)
Relevant automated test added (if you find this hard, leave it and we'll help out)
All tests run green

Documentation

User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)
Changelog is updated with a separate linked PR
- altinn-app-frontend- (if applicable)
- nuget-packages - (if applicable)

lorang92 · 2021-12-21T12:52:08Z

/AzurePipelines run

azure-pipelines · 2021-12-21T12:52:13Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

lorang92

Good stuff 🦖 I have some minor comments and a "question".

...tinn.Apps/AppFrontend/react/altinn-app-frontend/__tests__/utils/conditionalRendering.test.ts

src/Altinn.Apps/AppFrontend/react/altinn-app-frontend/src/components/GenericComponent.tsx

src/Altinn.Apps/AppFrontend/react/altinn-app-frontend/src/components/base/TextAreaComponent.tsx

src/Altinn.Apps/AppFrontend/react/altinn-app-frontend/src/components/index.ts

haakemon

Nice cleanup! Thanks for the improvements!

Would like to clarify the change for index as key (see other comment) before approval.

...pps/AppFrontend/react/altinn-app-frontend/__tests__/components/base/ButtonComponent.test.tsx

...pFrontend/react/altinn-app-frontend/__tests__/features/form/data/submitFormDataSagas.test.ts

...tinn.Apps/AppFrontend/react/altinn-app-frontend/__tests__/utils/conditionalRendering.test.ts

src/Altinn.Apps/AppFrontend/react/shared/src/components/molecules/AltinnMobileTableItem.tsx

Explains better how the function is used. Also fix some minor type issues. New interface IComponentProps Previously all components had their own interface. It is better when all share the common props from GenericComponent ``` export interface IComponentProps extends IGenericComponentProps { handleDataChange: (value: string, key?:string) => void, handleFocusUpdate: (componentId: string, step?: number) => void, getTextResource: (key: string) => React.ReactNode, getTextResourceAsString: (key: string) => string, formData: any, isValid: boolean, language: any, shouldFocus: boolean, text: React.ReactNode, label: ()=>JSX.Element, legend: ()=>JSX.Element, }; ``` Make formData type stable and use .simpleBinding everywhere Use React.ReactNode as type for getTextResource Apply formatting corrections from code review Co-authored-by: Steffen Lorang Ekeberg <steffen.ekeberg@gmail.com> Use a proper key for AltinnMobileTable Rename componentRendersItsOwnValidationErrors => componentValidationsHandledByGenericComponent Fix missing `?.simpleBinding` in Components Remove ITextAreaComponentProps to fix @typescript-eslint/no-empty-interface

ivarne · 2022-01-04T07:56:21Z

I fixed merge conflicts with #7721 and squashed commits.

lorang92 · 2022-01-18T09:28:36Z

@jeevananthank would you run regression tests for this branch?

lorang92 · 2022-01-18T09:29:22Z

/azp run

azure-pipelines · 2022-01-18T09:29:28Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

ivarne · 2022-01-18T10:40:03Z

@jeevananthank I took the freedom to merge #7843 into this, so that I likely would get my first "green PR"

jeevananthank · 2022-01-18T10:55:54Z

@ivarne the failed test is the same as i mentioned here

ivarne · 2022-01-18T10:59:39Z

Yea, I can see that, but strangely the change I picked out as responsible is not part of this PR. I'll need too look into it later this week. Is the tests or screenshots available somewhere, so I can get some more insight into what is failing.

jeevananthank · 2022-01-18T11:04:11Z

Yea, I can see that, but strangely the change I picked out as responsible is not part of this PR. I'll need too look into it later this week. Is the tests or screenshots available somewhere, so I can get some more insight into what is failing.

you can find the test results here. Screenshots are not available for runs from fork PR (unfortunately to enable test runs on fork PR, we had to give up the recording feature)

ivarne · 2022-01-18T11:12:02Z

Is there anything more than this screenshot I should look at in the log you linked to?

jeevananthank · 2022-01-18T11:53:37Z

@ivarne no, you just get to see the test results in the logs of a github action run. from the name of the test failed, you can find the test script here

ivarne · 2022-01-19T11:32:12Z

Yey, 💚

So much time wasted on the assumption that this was a different issue than the one in #7843, and a quick look at the diff to (wrongly) confirm that this change was not part of the first PR.

Anyway. This is green now. 🥳 🎉

jeevananthank · 2022-01-19T11:48:41Z

@jeevananthank would you run regression tests for this branch?

automated tests have run green 😀

lorang92 · 2022-01-20T07:23:15Z

/azp run

azure-pipelines · 2022-01-20T07:23:20Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

lorang92 · 2022-01-20T07:31:42Z

/azp run

azure-pipelines · 2022-01-20T07:31:46Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

lorang92 · 2022-01-20T07:56:06Z

Yey, 💚

So much time wasted on the assumption that this was a different issue than the one in #7843, and a quick look at the diff to (wrongly) confirm that this change was not part of the first PR.

Anyway. This is green now. 🥳 🎉

@ivarne and merged ❤️ much appreciated!

github-actions bot added the solution/app-frontend label Dec 21, 2021

lorang92 suggested changes Dec 21, 2021

View reviewed changes

haakemon reviewed Dec 21, 2021

View reviewed changes

haakemon mentioned this pull request Dec 21, 2021

fix: some hooks had incorrect dependencies specified, which could cau… #7722

Closed

8 tasks

github-actions bot added the area/instantiation label Dec 21, 2021

lorang92 added the community-contribution-❤️ Contributions from developers outside the Altinn teams. label Dec 22, 2021

ivarne force-pushed the formData-typeStable branch from 5ddebd5 to 25c8529 Compare January 4, 2022 07:55

This was referenced Jan 7, 2022

Checkbox improvements #7775

Closed

Analsysis: Should we allow (in a standardised way) checkbox support for individual data model fields for each alternative Altinn/app-frontend-react#789

Open

haakemon approved these changes Jan 17, 2022

View reviewed changes

ivarne and others added 2 commits January 17, 2022 17:54

Merge branch 'master' into formData-typeStable

d0fd804

Merge remote-tracking branch 'origin/master' into formData-typeStable

b16227c

jeevananthank added 2 commits January 18, 2022 11:35

support for running test on external pr

0cbf507

added missing quote

1e9b012

github-actions bot added the area/test label Jan 18, 2022

Fix broken test, but keep the strange test.

54b897a

lorangSec added 2 commits January 20, 2022 08:35

Merge remote-tracking branch 'origin/master' into formData-typeStable

bb41474

chore: bump package.json

dfcd15e

lorang92 approved these changes Jan 20, 2022

View reviewed changes

lorang92 merged commit ddb259c into Altinn:master Jan 20, 2022

ivarne deleted the formData-typeStable branch February 9, 2025 07:25

ivarne restored the formData-typeStable branch February 9, 2025 07:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make FormData type stable, and add/improve lots of related Typescirpt typings #7718

Make FormData type stable, and add/improve lots of related Typescirpt typings #7718

ivarne commented Dec 21, 2021

lorang92 commented Dec 21, 2021

azure-pipelines bot commented Dec 21, 2021

lorang92 left a comment

haakemon left a comment

ivarne commented Jan 4, 2022

lorang92 commented Jan 18, 2022

lorang92 commented Jan 18, 2022

azure-pipelines bot commented Jan 18, 2022

ivarne commented Jan 18, 2022

jeevananthank commented Jan 18, 2022

ivarne commented Jan 18, 2022

jeevananthank commented Jan 18, 2022 •

edited

Loading

ivarne commented Jan 18, 2022

jeevananthank commented Jan 18, 2022

ivarne commented Jan 19, 2022

jeevananthank commented Jan 19, 2022

lorang92 commented Jan 20, 2022

azure-pipelines bot commented Jan 20, 2022

lorang92 commented Jan 20, 2022

azure-pipelines bot commented Jan 20, 2022

lorang92 commented Jan 20, 2022

Make FormData type stable, and add/improve lots of related Typescirpt typings #7718

Make FormData type stable, and add/improve lots of related Typescirpt typings #7718

Conversation

ivarne commented Dec 21, 2021

Description

Fixes

Verification

Documentation

lorang92 commented Dec 21, 2021

azure-pipelines bot commented Dec 21, 2021

lorang92 left a comment

Choose a reason for hiding this comment

haakemon left a comment

Choose a reason for hiding this comment

ivarne commented Jan 4, 2022

lorang92 commented Jan 18, 2022

lorang92 commented Jan 18, 2022

azure-pipelines bot commented Jan 18, 2022

ivarne commented Jan 18, 2022

jeevananthank commented Jan 18, 2022

ivarne commented Jan 18, 2022

jeevananthank commented Jan 18, 2022 • edited Loading

ivarne commented Jan 18, 2022

jeevananthank commented Jan 18, 2022

ivarne commented Jan 19, 2022

jeevananthank commented Jan 19, 2022

lorang92 commented Jan 20, 2022

azure-pipelines bot commented Jan 20, 2022

lorang92 commented Jan 20, 2022

azure-pipelines bot commented Jan 20, 2022

lorang92 commented Jan 20, 2022

jeevananthank commented Jan 18, 2022 •

edited

Loading