Massive cleanup in component rendering/generation #976
Conversation
…know some tests broke now, probably a lot more.
… to fetch the node object
…s when calling GenericComponent
…ead of relying on straight up component properties
# Conflicts: # src/components/summary/SummaryComponent.tsx # src/components/summary/SummaryComponentSwitch.tsx
… that was passed around everywhere in the Summary components
…ponent type. This way we can also check for specific component types using `obj instanceof FormComponent` and implement form-component specific functionality (like having to support a summary)
…ry-refactor` branch
…summary/repeating group table data as react hooks. The latter removes the need for SummaryContext, so I'm removing that.
…d looks somewhat like the code in `main`. Still some inconsistencies, though.
…t component display data in repeating groups
…This is now just as broken as it used to be, but at least I didn't break anything even more.
…there are no rows in a repeating group), otherwise it was just streamlining DOM IDs and testids.
…w the table is not rendered at all when there are no rows to display, so we cannot assert there are 0-length rows, but rather have to assert that the entire table does not exist.
…ey were just needlessly difficult to fix, and provides little to no value (I've made a note of these to make sure we write Cypress tests for them instead).
olemartinorg
added
the
kind/other
olemartinorg
requested review from
bjosttveit,
Magnusrm,
framitdavid and
lassopicasso
and removed request for
bjosttveit
🤯🤯🤯🤯 |
| const value = node.item.dataModelBindings?.simpleBinding | ||
| ? formData[node.item.dataModelBindings.simpleBinding] || '' | ||
| : ''; | ||
| return useCommaSeparatedOptionsToText(node.item, value); |
There was a problem hiding this comment.
The project has a folder named hooks, hence I think hooks should be created within the hooks folder instead or I might misunderstand the architecture. The useCommaSeparatedOptionsToText method is imported from the utils folder. In my head when I'm thinking of utility methods they do not have an internal state like useCommaSeparatedOptionsToText has. Utility methods should only depend on the inputs and return the same result as long the inputs are the same.
What do you think about my thoughts? I'm new on the team so I have might misunderstand some standards within this project. But I'm still thinking of utilities as static functions that only depend on their input. :D
There was a problem hiding this comment.
I agree completely! Will move these hooks into the hooks folder.
|
|
||
| return uuids | ||
| .map((uuid) => { | ||
| const attachmentsForComponent = attachments[componentId]; | ||
| if (attachmentsForComponent) { | ||
| const foundAttachment = attachmentsForComponent.find((a) => a.id === uuid); | ||
| if (foundAttachment && foundAttachment.name) { | ||
| return foundAttachment; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }) | ||
| .filter((a) => a !== null) as IAttachment[]; |
There was a problem hiding this comment.
What do you think about refactoring a little to avoid filtering null and casting the type? Would it be a little more readable to follow the suggestion? The suggestion has moved attachmentsForComponent outside the loop to avoid creating and assigning on every iteration. :)
| return uuids | |
| .map((uuid) => { | |
| const attachmentsForComponent = attachments[componentId]; | |
| if (attachmentsForComponent) { | |
| const foundAttachment = attachmentsForComponent.find((a) => a.id === uuid); | |
| if (foundAttachment && foundAttachment.name) { | |
| return foundAttachment; | |
| } | |
| } | |
| return null; | |
| }) | |
| .filter((a) => a !== null) as IAttachment[]; | |
| const attachmentsForComponent = attachments[componentId]; | |
| if (!attachmentsForComponent) { | |
| return []; | |
| } | |
| let componentAttachments: IAttachment[] = []; | |
| uuids.forEach((uuid) => { | |
| const foundAttachment = attachmentsForComponent.find((a) => a.id === uuid); | |
| if (foundAttachment?.name) { | |
| componentAttachments = [...componentAttachments, foundAttachment]; | |
| } | |
| }); | |
| return componentAttachments; |
There was a problem hiding this comment.
Finally looked at closely at this, and yup, that's much better! I took the liberty to rewrite it to use for (const uuid of uuids) and componentAttachments.push() as well. 🥳 Thank you!
There was a problem hiding this comment.
No worries! Sounds great to use the push method, then componentAttachments can be const instead of let. Great work! 🥳
| { | ||
| "files": ["src/layout/*/index.tsx"], | ||
| "rules": { | ||
| "react-hooks/rules-of-hooks": "off" |
There was a problem hiding this comment.
It feels a little strange/painful to have to turn this rule off, but as long we are using classes I don't think this is an issue. It might be a sign that we should implement our code differently, but I'm not sure. The rule is mainly used to ensure that hooks are initialized in the top scope within the functional component, hence for classes this rule does not make sense.
There was a problem hiding this comment.
Yup, I know.. 😆 Little on the fence on this one as well. I guess we could separate out the methods that are meant to be used as hooks, but I'm not sure I like that approach either.
There was a problem hiding this comment.
Sounds like a pair-programming issue to me, to discuss the different approaches that could be implemented. Anyway, this is not a task for this PR. 😃
# Conflicts: # src/components/summary/SingleInputSummary.tsx
…but the change is so large that git thinks the original was deleted)
|
SonarCloud Quality Gate failed.
|
…ere I broke this functionality, and rewrote the test). The table is now in the DOM, but has no elements in it.
Description
This PR includes an overhaul to the way we render layout components. Until now, we've had the legacy code that generates component properties, resolves data model bindings/mapping/text resource bindings for repeating groups, and in the end injects these properties into
GenericComponentto render it. After this PR, some things will change under the hood:GenericComponent, anodeobject is passed into it. You can no longer render imaginary components that do not belong anywhere in the node hierarchy (and by extension, exist inside the redux store). This makes sure we develop new functionality with the node hierarchy and dynamic expressions in mind.LayoutComponentclass has now been split into 3:PresentationComponent,FormComponentandActionComponent(for buttons, navigation). These 3 component types may get different functionality in the future, and make sure we implement the important parts for every component type (i.e. we have to implement a function to get the component data to display as a string in the repeating group table, and every form component also needs to be able to render itself as a Summary item).Summaryhas been majorly refactored in this PR, and now relies on the node hierarchy throughout. As mentioned, every component now decide for themselves how it should be rendered in a Summary.Related Issue(s)
Verification/QA
src/layout/layout.d.tsandlayout.schema.v1.json, and these are all backwards-compatiblekind/*label to this PR for proper release notes grouping