Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Architecture: Modularize components using classes and inheritance #377

Closed
Tracked by #1660
olemartinorg opened this issue Aug 10, 2022 · 1 comment
Closed
Tracked by #1660
Assignees
Labels
Epic fe-v4 Issues to be solved before v4 goes gold kind/analysis quality/debt

Comments

@olemartinorg
Copy link
Contributor

olemartinorg commented Aug 10, 2022

Description

There are multiple code paths which branch off to do something different depending on the encountered component type. This has historically been needed to implement some custom functionality for "abnormal" components.

Solution

The idea in this issue is to refactor away some complexities in existing code such that we can have cleaner implementations of the generic frontend app, and at the same time concentrate custom overrides for each component in their respective folders. This should also make it easier to implement a new component, as potential overrides are centralized. One way to implement this is to define an abstract class (i.e. BaseComponent), have every component implement their own concrete class from this abstract base class, and then reference instances of these classes in the components map in src/altinn-app-frontend/src/components/index.ts. GenericComponent.tsx can then fetch each react component via components[props.type].getReactComponent() or similar.

Examples from GenericComponent.tsx

This case should instead be handled using a function call, like !component.shouldRenderValidationsGenerically() (quite possibly an overly verbose method name).

const getValidationsForInternalHandling = () => {
  if (
    props.type === 'AddressComponent' ||
    props.type === 'DatePicker' ||
    props.type === 'FileUpload' ||
    props.type === 'FileUploadWithTag' ||
    (props.type === 'Likert' && props.layout === LayoutStyle.Table)
  ) {
    return componentValidations;
  }
  return null;
};

// some components handle their validations internally (i.e merge with internal validation state)
const internalComponentValidations = getValidationsForInternalHandling();
if (internalComponentValidations !== null) {
  passThroughProps.componentValidations = internalComponentValidations;
}

This case should instead be handled using a function call, like component.shouldRenderLabel():

const noLabelComponents = [
  'Header',
  'Paragraph',
  'Image',
  'Submit',
  'ThirdParty',
  'AddressComponent',
  'Button',
  'Checkboxes',
  'RadioButtons',
  'AttachmentList',
  'InstantiationButton',
  'NavigationBar',
  'Likert',
  'Panel',
];

Examples from validation.ts:

In validateEmptyFieldsForLayout, this code should instead look up the function to see if it has implemented any custom 'empty field' validations. Quite possibly every component type should have its own method to figure out if the formData is "empty" or not.

if (
  component.type === 'FileUpload' ||
  component.type === 'FileUploadWithTag'
) {
  // These components have their own validation in validateFormComponents(). With data model bindings enabled for
  // attachments, the empty field validations would interfere.
  continue;
}

The whole function called validateFormComponentsForLayout is filled with custom validation rules for certain components. These should be implemented in the component definitions themselves.

In findLayoutIdsFromValidationIssue(), there is a special case for some components:

// Special handling for FileUpload components
if (c.type === 'FileUpload' || c.type === 'FileUploadWithTag') {
  return c.id === validationIssue.field;
}

Examples in formComponentUtils.ts:

The functions componentValidationsHandledByGenericComponent, getDisplayFormData and getFormDataForComponentInRepeatingGroup include lots of code to make sure different components produce some valid results. These should look up some component definition/declaration instead, where components themselves can override implementations.

@olemartinorg olemartinorg mentioned this issue Nov 8, 2022
4 tasks
@bjosttveit bjosttveit moved this to 📈 Todo in Team Apps Nov 24, 2022
@olemartinorg olemartinorg self-assigned this Dec 13, 2022
@olemartinorg olemartinorg moved this from 📈 Todo to 👷 In Progress in Team Apps Dec 13, 2022
@olemartinorg olemartinorg moved this from 👷 In Progress to 📈 Todo in Team Apps Dec 19, 2022
@olemartinorg olemartinorg moved this from 📈 Todo to 👷 In Progress in Team Apps Dec 29, 2022
@olemartinorg olemartinorg mentioned this issue Dec 29, 2022
14 tasks
@olemartinorg olemartinorg moved this from 👷 In Progress to 📈 Todo in Team Apps Jan 20, 2023
@olemartinorg olemartinorg moved this from 📈 Todo to 👷 In Progress in Team Apps Mar 3, 2023
@olemartinorg
Copy link
Contributor Author

Closing this epic, as the concept has now been well-understood, and we're a long way from were we were when this was first created. With v4 we're going further with modularization than we've done before, and even though there are still things we could solve better, I think it's better to focus on that in code review and future discussions rather than leaving this Epic open without having any measurable and concrete things to do with it.

@olemartinorg olemartinorg moved this from 👷 In Progress to 🧪 Test in Team Apps Jan 26, 2024
@RonnyB71 RonnyB71 moved this from 🧪 Test to ✅ Done in Team Apps Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic fe-v4 Issues to be solved before v4 goes gold kind/analysis quality/debt
Projects
Status: Done
Archived in project
Development

No branches or pull requests

2 participants