Altinn / app-frontend-react

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

Feat/1285 accordion and accordion group #1301

Closed mikaelrss closed 10 months ago

mikaelrss commented 11 months ago

Description

Adds two new components, Accordion and AccordionGroup.

Related Issue(s)

Verification/QA

bjosttveit commented 11 months ago

Does this solve https://github.com/Altinn/app-frontend-react/issues/165?

mikaelrss commented 11 months ago

I guess it could solve #165. As of right now, the Accordion only supports Paragraph and Button components as children, and it seems like the issue needs form components to be placed inside an Accordion. I guess we could expand to accompany for more components.

mikaelrss commented 10 months ago

SonarCloud is complaining about code coverage, but I cannot see how I would add any meaningful tests to the new code I've introduced. Do you have any suggestions?

It also complains about some duplication of code, but I am not sure creating an abstraction that would share code between the two hierarchy.ts-files in Accordion and AccordionGroup would be the best idea. What do you think?

There's also some screenshots that are failing in Percy. Most of these are the new Alert and Accordion components that have not yet run on main. There also seems to be an issue with nubering lables in the Summary part of the test where a .2 is missing in the original screenshots:

Original: image New: image

olemartinorg commented 10 months ago

SonarCloud is complaining about code coverage, but I cannot see how I would add any meaningful tests to the new code I've introduced. Do you have any suggestions?

My suggestion is don't worry about it. 😊 We don't care that much about the sonarcloud coverage. But right now there's probably no coverage over your hiearchy generator etc, so maybe a full integration test for these components are in order? It might be difficult to do with pure unit tests, but easy to observe the effects of it in a Cypress test.

It also complains about some duplication of code, but I am not sure creating an abstraction that would share code between the two hierarchy.ts-files in Accordion and AccordionGroup would be the best idea. What do you think?

I agree, ignore it.

There's also some screenshots that are failing in Percy. Most of these are the new Alert and Accordion components that have not yet run on main. There also seems to be an issue with nubering lables in the Summary part of the test where a .2 is missing in the original screenshots:

Not sure about that problem, maybe @bjosttveit knows? I assumed it was some CSS changes he made recently.

bjosttveit commented 10 months ago

There's also some screenshots that are failing in Percy. Most of these are the new Alert and Accordion components that have not yet run on main. There also seems to be an issue with nubering lables in the Summary part of the test where a .2 is missing in the original screenshots:

Not sure about that problem, maybe @bjosttveit knows? I assumed it was some CSS changes he made recently.

This exact difference has been happening to me for weeks, I don't know why. When checking manually I have never seen this occur. It always seems to be the old screenshot that gets messed up so I usually ignore this difference.

Checking an old version using dev tools:

v3.65.1:

image

main:

image
mikaelrss commented 10 months ago

But right now there's probably no coverage over your hiearchy generator etc, so maybe a full integration test for these components are in order? It might be difficult to do with pure unit tests, but easy to observe the effects of it in a Cypress test.

I have already added one cypress test testing that the accordion opens and closes, and that the content is invisible/visible. This should cover Accordion/hierarchy.ts right? It also covers AccordoinGroup/hierarchy.ts since frontend-test includes an accordion group as well. However, I haven't made any assertions to specifically test AccordionGroup, as this is mainly a style change for Accordions.

@olemartinorg Do you think this is enough coverage, or is there something I am missing?

sonarcloud[bot] commented 10 months ago

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

36.6% 36.6% Coverage
15.6% 15.6% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

olemartinorg commented 10 months ago

@mikaelrss Sorry for missing the test! 🙈 Yup, sounds like more than enough tests, then. 🙏

@bjosttveit Ah! I changed something in frontend-test a while ago that might affect that. In the text resource, we didn't escape the 2. so markdown formatting thought it a numbered list item. That might explain parts of it, I think. It seems to me that Percy lags behind a while after there has been a change, so I guess we should just expect there to be large diffs a while after a large change... :-/