Add support for labelGrid #7027

ivarne · 2021-10-01T11:54:00Z

innerGrid lets you adjust the size of form fields when the expected content is short. Here I provide a way to set labelGrid in addition to enable long lists of short questions and answers to be presented in a table like way to save screen space.

TODO

Bump version in src/Altinn.Apps/AppFrontend/react/altinn-app-frontend/package.json
Update JSON schema https://altinncdn.no/schemas/json/layout/layout.schema.v1.json Added Json schema for labelGrid altinn-cdn#56
Create Docs PR Add some documentation about labelGrid altinn-studio-docs#310

Sample

{
  "id": "3-2-1-1",
  "type": "Input",
  "textResourceBindings": {
    "title": "3-2.1.1",
    "help": "3-2.1.1.hjelpetext"
  },
  ...
  "grid": {
    "innerGrid":{
      "md": 4
    },
    "labelGrid":{
      "md": 8
    }
  }
},

krt-.krt-1006-inkasso.-.Google.Chrome.2021-10-01.13-17-28_Trim.mp4

innerGrid lets you adjust the size of form fields when the expected content is short. Here I provide a way to set labelGrid in order to enable long lists of short questions and answers to be presented in a table like way to save screen space.

lorang92

Neat! If you have a use case for this I see no reason not to support this grid prop. Agree @Febakke ?

The version defined in src/Altinn.Apps/AppFrontend/react/altinn-app-frontend/package.json is used for versioning of the app-frontend application which is hosted at our cdn. This version should therefore be bumped.

We should also document this in a similar way as innerGrid. If ux agrees that this is something we want to go with.

Febakke · 2021-10-01T13:03:26Z

Cool! 🔥
One thing we need to fix is the distance between the labels and the fields. I cant immediately see a solution that will work for every case as it will depend so much on the length of the label. Best way to solve this might is trough guidelines/documentation on how to best use this functionality? @lorang92

Why:
As it is in the example it is harder to scan with the smaller labels than the long ones and is breaking WCAG Success Criterion 3.3.2: Labels or Instructions. To fix this labels should be placed immediately to left off the field.

ivarne · 2021-10-01T13:11:31Z

I don't think it is possible to really ensure WCAG success with shorter labels. (without gridlines) A 6-6 split instead of 8-6 would probably be somewhat better in this example, but I changed it to have 1 fewer label with 2 lines of text.

ivarne · 2021-10-01T13:14:58Z

We can probably add something to the docs about long gaps that might lead to failing a WCAG success criteria.

Theese are ignored in PartySelection as well, and are really annoying when they break altinn-studio\src\Altinn.Apps\AppFrontend\react\altinn-app-frontend> npm start

Febakke · 2021-10-01T13:32:40Z

I don't think it is possible to really ensure WCAG success with shorter labels. (without gridlines) A 6-6 split instead of 8-6 would probably be somewhat better in this example, but I changed it to have 1 fewer label with 2 lines of text.

Then I have to admit Im a little sceptical of this solution. Label size should not be deciding if something is accessible or not.

ivarne · 2021-10-01T13:49:53Z

Regarding the risk that a combination of label sizes and split might make it possible to not meet all WCAG success criteria.

This is not the default layout.
A user will have to explicitly pick this layout and set the split
It will generally be used when there are many questions in order and to conserve some space.
For financial data users will be very accustomed to tables and views approximating excel. Breaking that pattern is actually worse for readability.

altinnadmin · 2021-10-04T05:56:21Z

I think adding good docs and guidelines as suggested would make this a logical and simple expansion of our grid support. This is NICE! 👍

ivarne · 2021-10-04T06:03:50Z

If the grid is defined at a size greater than xs, a user with high zoom will not get the side by side layout.

Febakke · 2021-10-05T06:47:57Z

We have proposal that might mitigate the readablity and WCAG issues. A horizontal line between the fields would give a visual indication between field and label.

Detailed specification in Figma

ivarne · 2021-10-05T07:04:02Z

Great!

I see more than one option for how to implement it in layout.json?

Create a new custom property for horizontal layout that can be used as an alternative to grid (which adds the underline)
Take the current proposal and add a new underline property to add an underline below the question.
- Possibilities of users not wanting (or forgetting) the underline.
- Should users be able to pick dotted, dashed, solid, hairline?
Create a custom Group element with horizontal layout.
(added) Create the underline automatically if labelGrid is set

It is nice to have some flexibility about where on the screen the split should be (sometimes the labels are long and answers are very short, and sometimes opposite.)

ivarne · 2021-10-09T12:34:47Z

@Febakke After some thinking I think I found out how to implement this, just adding the lines automatically under each field, based on the same breakpoints as specified in labelGrid.

krt-.krt-1006-inkasso.-.Google.Chrome.2021-10-09.14-26-26_Trim_Trim.mp4

PS: I used pure CSS because I don't know material very well, and we seem to be in the middle of a switch from 4-5 anyway.
(see Altinn/app-frontend-react#174)

Altinn/altinn-studio#7027

mjulstein · 2021-10-19T07:33:13Z

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

      </Grid>
-    </Grid>
+      <hr className={gridToHiddenProps(props.grid?.labelGrid)} />


What is the purpose of this hr?

Gridline for users that might have issues figuring out what label is for which field🧑‍🦯

OK, as you are using media-query anyway, could you maybe just style border on the Grid instead of adding more complexity to the DOM?
On line 47, 59 and 241 is an example of how to use media-query and css with Mui.
This is the preferred method, as the library compiles the css classes with hashes and thereby preventing other components from accidentally get impacted.
@Febakke do you want the last element in the list to have a dotted line as well?

I could definetly style border-bottom on the Grid instead. I'm not sure what is best.

I started to use media queries with MUI, but there are quite extensive changes from MUI 4 to MUI 5, and https://github.com/Altinn/altinn-studio/issues/7047 indicates that making it work in MUI 4 is kind of wasted time.

I think it would help to reduce the extra DOM elements.

If one way over the other would remove the need to revisit the component when upgrading to MUI 5 I would go for that. I can't find where MUI 5 is going to stop supporting how we do media queries today.

@mjulstein
Yes, it makes sense to me that all fields have a line at the bottom including the last one. You can see the sketch here: #7027 (comment)

I was also curious about the extra line at the bottom of the last one, but I can't really see how to write a condition on the flow of the next item, so I think it is requried (unless we add some preprocessing).

@mjulstein I tried <Hidden which will be gone, and sx which is not availible yet. Pure jss will likely work, but I was tired after two failiures. Maybe the css->jss translation is simple. I'll try and report back.

MUI is also changing its style library from styled-components to emotion, but reading that again, it seems like we will keep using the old way, so that is a much smaller issue than I thought.

mjulstein · 2021-10-19T07:33:16Z

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

@@ -291,4 +303,15 @@ const RenderLabelScoped = (props: IRenderLabelProps) => {
  );
 };

+const gridToHiddenProps = (props?: IGridStyling) => {
+  return classNames({


simplify this by checking props before every value. Can return props && ... instead.

I'm not sure I understand. Can you elaborate?

const gridToHiddenProps = (props?: IGridStyling) => { return props && classNames({ hrLabelGrid: !!props, xs: props.xs > 0 && props.xs < 12, sm: props.sm > 0 && props.sm < 12, md: props.md > 0 && props.md < 12, lg: props.lg > 0 && props.lg < 12, xl: props.xl > 0 && props.xl < 12, }); };

There is no need to test props on every line, and undefined is better than empty string in className as it should prevent lines that appear as <hr class /> to appear as <hr />

instead of a separate <hr> tag

ivarne · 2021-10-19T13:50:12Z

@mjulstein I think all issues should be fixed now. Would you mind taking another look?

mjulstein · 2021-10-21T12:50:54Z

@ivarne nice changes! Sorry that I didn't reply sooner.
I'm not sure why the Cypress tests are failing maybe @lorang92 has more insight? Or we might have to wait for @jeevananthank to return.
I'm not very familiar with the Altinn.Apps space so I can only post suggestions on code-smells for now.

ivarne · 2021-10-21T12:52:32Z

I think it is due to some secrets not available when I have a pull from a fork.

altinnadmin · 2021-10-22T05:45:08Z

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

@mjulstein Are you able to trigger a run manually?
@jeevananthank Have you considered using pull_request_target? I changed to that trigger for our labeler action to make it work for forks.

mjulstein · 2021-10-22T11:39:20Z

@altinnadmin

lorang92 · 2021-11-16T09:21:03Z

@mjulstein @ivarne so sorry for the long delay. I must have missed that I was tagged in anything here... Yeah, we need to manually run the cypress tests since we don't allow secrets on forks, we can do this when we are testing/confirming the change so that won't be a problem.

@ivarne I assume this change is still something you still need? If we could resolve the merge conflict we could get our internal tester to take a look at this PR and hopefully get up to speed again.

ivarne · 2021-11-16T10:01:20Z

@lorang92 I think this branch is ok now. Just too bad I'm working on too many branches at once so I start working on making that a little easier, but then i just end up with 2 new branches.

I'm a litte lost testing the finished merge, because of some unrelated build errors

lorang92 · 2021-11-16T10:09:36Z

@lorang92 I think this branch is ok now. Just too bad I'm working on too many branches at once so I start working on making that a little easier, but then i just end up with 2 new branches.

I'm a litte lost testing the finished merge, because of some unrelated build errors

Thank you 👏 These build errors do seem to be related to our new lint rule setup (#7387), are you aware of any issues here @haakemon?

haakemon · 2021-11-16T10:32:34Z

Hm, thats odd... The error shouldnt appear for this. Ill have a look.

jeevananthank · 2021-11-16T14:49:35Z

regression test results for app frontend

lorang92 · 2021-11-18T06:20:51Z

@jeevananthank @ivarne neat. Draggeg this issue to test now, @jeevananthank could you do a verification of the functionality described here Altinn/altinn-studio-docs#310

jeevananthank · 2021-11-19T10:47:44Z

test is complete.

lorang92 · 2021-11-29T11:06:56Z

@ivarne this is finally ready for merge. Would you mind merging with master and bump package.json? 👼

ivarne · 2021-11-29T11:12:34Z

Done

lorang92 · 2021-11-29T11:17:40Z

/AzurePipelines run

azure-pipelines · 2021-11-29T11:17:45Z

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.

src/Altinn.Apps/AppFrontend/react/altinn-app-frontend/package.json

…json

ivarne · 2021-11-29T13:11:16Z

Could you look at Altinn/altinn-cdn#56 as well?

* Added Json schema for labelGrid Altinn/altinn-studio#7027 * Update schemas/json/layout/layout.schema.v1.json Co-authored-by: Steffen Lorang Ekeberg <steffen.ekeberg@gmail.com> * Fix indentation levels in layout.schema.v1.json. Co-authored-by: Steffen Lorang Ekeberg <steffen.ekeberg@gmail.com>

Add support for labelGrid

f8856c5

innerGrid lets you adjust the size of form fields when the expected content is short. Here I provide a way to set labelGrid in order to enable long lists of short questions and answers to be presented in a table like way to save screen space.

github-actions bot added the solution/app-frontend label Oct 1, 2021

jeevananthank added the external-contribution-❤️ Pull request from a developer outside the Altinn teams. label Oct 1, 2021

lorang92 reviewed Oct 1, 2021

View reviewed changes

// eslint-disable-next-line react/jsx-no-bind

b6920d0

Theese are ignored in PartySelection as well, and are really annoying when they break altinn-studio\src\Altinn.Apps\AppFrontend\react\altinn-app-frontend> npm start

Merge branch 'eslint-ignore' into labelGrid

d38c4f3

ivarne added 2 commits October 9, 2021 08:46

Merge remote-tracking branch 'origin/master' into labelGrid

4679f34

Add underline when labelGrid is in use.

761b1fc

ivarne mentioned this pull request Oct 10, 2021

Add some documentation about labelGrid Altinn/altinn-studio-docs#310

Merged

ivarne added a commit to ivarne/altinn-cdn that referenced this pull request Oct 10, 2021

Added Json schema for labelGrid

5a96705

Altinn/altinn-studio#7027

This was referenced Oct 10, 2021

Added Json schema for labelGrid Altinn/altinn-cdn#56

Merged

Add support for formatting.alignRight on Input components #7034

Merged

mjulstein reviewed Oct 19, 2021

View reviewed changes

ivarne added 3 commits October 19, 2021 14:51

Merge branch 'master' into labelGrid

e294b2d

Use jss instead of css styling, and use border-bottom on Grid

11837de

instead of a separate <hr> tag

remove obsolete fragment.

d67d04f

ivarne force-pushed the labelGrid branch from d67d04f to 16a9625 Compare November 16, 2021 09:36

Merge remote-tracking branch 'origin/master' into labelGrid

7fdd5a6

ivarne force-pushed the labelGrid branch from 16a9625 to 7fdd5a6 Compare November 16, 2021 09:46

RonnyB71 assigned lorang92 Nov 17, 2021

RonnyB71 added this to the 2021 - W46/47 milestone Nov 17, 2021

jeevananthank self-assigned this Nov 18, 2021

jeevananthank removed their assignment Nov 19, 2021

ivarne added 2 commits November 29, 2021 12:09

Merge remote-tracking branch 'origin/master' into labelGrid

55188b2

bump frontend version

8600cb6

lorang92 suggested changes Nov 29, 2021

View reviewed changes

src/Altinn.Apps/AppFrontend/react/altinn-app-frontend/package.json Outdated Show resolved Hide resolved

Update src/Altinn.Apps/AppFrontend/react/altinn-app-frontend/package.…

527ce78

…json

lorang92 approved these changes Nov 29, 2021

View reviewed changes

lorang92 merged commit eaf151d into Altinn:master Nov 29, 2021

ivarne mentioned this pull request Jun 3, 2022

Make labelGrid work for radio buttons. Altinn/app-frontend-react#212

Closed

4 tasks

Add support for labelGrid #7027

Add support for labelGrid #7027

Conversation

ivarne commented Oct 1, 2021 • edited by jeevananthank Loading

TODO

Sample

lorang92 left a comment • edited Loading

Choose a reason for hiding this comment

Febakke commented Oct 1, 2021

ivarne commented Oct 1, 2021

ivarne commented Oct 1, 2021

Febakke commented Oct 1, 2021

ivarne commented Oct 1, 2021

altinnadmin commented Oct 4, 2021

ivarne commented Oct 4, 2021

Febakke commented Oct 5, 2021

ivarne commented Oct 5, 2021 • edited Loading

ivarne commented Oct 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjulstein Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjulstein Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivarne commented Oct 19, 2021

mjulstein commented Oct 21, 2021

ivarne commented Oct 21, 2021

altinnadmin commented Oct 22, 2021

mjulstein commented Oct 22, 2021

lorang92 commented Nov 16, 2021

ivarne commented Nov 16, 2021 • edited Loading

lorang92 commented Nov 16, 2021

haakemon commented Nov 16, 2021

jeevananthank commented Nov 16, 2021

lorang92 commented Nov 18, 2021

jeevananthank commented Nov 19, 2021

lorang92 commented Nov 29, 2021

ivarne commented Nov 29, 2021

lorang92 commented Nov 29, 2021

azure-pipelines bot commented Nov 29, 2021

ivarne commented Nov 29, 2021

ivarne commented Oct 1, 2021 •

edited by jeevananthank

Loading

lorang92 left a comment •

edited

Loading

ivarne commented Oct 5, 2021 •

edited

Loading

mjulstein Oct 19, 2021 •

edited

Loading

mjulstein Oct 19, 2021 •

edited

Loading

ivarne commented Nov 16, 2021 •

edited

Loading