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

Feature/table #2593

Open
wants to merge 66 commits into
base: main
Choose a base branch
from
Open

Feature/table #2593

wants to merge 66 commits into from

Conversation

adamhaeger
Copy link
Contributor

@adamhaeger adamhaeger commented Oct 17, 2024

Description

  • Adds Table component
  • Introduces component libary
  • Includes ADR describing the proposed change
Screenshot 2024-10-17 at 11 36 08 Screenshot 2024-10-17 at 14 17 44

Related Issue(s)

Verification/QA

  • Manual functionality testing
    • I have tested these changes manually
    • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
    • No testing done/necessary
  • Automated tests
    • Unit test(s) have been added/updated
    • Cypress E2E test(s) have been added/updated
    • No automatic tests are needed here (no functional changes/additions)
    • I want someone to help me make some tests
  • UU/WCAG (follow these guidelines until we have our own)
    • I have tested with a screen reader/keyboard navigation/automated wcag validator
    • No testing done/necessary (no DOM/visual changes)
    • I want someone to help me perform accessibility testing
  • User documentation @ altinn-studio-docs
    • Has been added/updated
    • No functionality has been changed/added, so no documentation is needed
    • I will do that later/have created an issue
  • Support in Altinn Studio
    • Issue(s) created for support in Studio
    • This change/feature does not require any changes to Altinn Studio
  • Sprint board
    • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
    • I don't have permissions to do that, please help me out
  • Labels
    • I have added a kind/* label to this PR for proper release notes grouping
    • I don't have permissions to add labels, please help me out

Copy link
Contributor

@cammiida cammiida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dette blir bra! 🚀

adr/001-component-library.md Outdated Show resolved Hide resolved
Copy link
Contributor

@paal2707 paal2707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good. Looking forward to having this on the rest of the component as well. Approved with some questions/comments.

src/app-components/table/Table.tsx Show resolved Hide resolved
src/app-components/table/caption/Caption.tsx Outdated Show resolved Hide resolved
src/features/formData/FormDataWriteStateMachine.tsx Outdated Show resolved Hide resolved
src/features/formData/FormDataWriteStateMachine.tsx Outdated Show resolved Hide resolved
src/layout/Table/TableComponent.tsx Show resolved Hide resolved
src/layout/Table/TableComponent.tsx Show resolved Hide resolved
Copy link
Contributor

@cammiida cammiida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vil ikke approve pga ekstern datamodell, men ser meget bra ut 💯

src/app-components/table/Table.tsx Outdated Show resolved Hide resolved
src/layout/Table/Table.module.css Outdated Show resolved Hide resolved
src/layout/Table/TableComponent.tsx Outdated Show resolved Hide resolved
src/layout/Table/TableComponent.tsx Outdated Show resolved Hide resolved
.extends(CG.common('LabeledComponentProps'))
.extendTextResources(CG.common('TRBLabel'))
.addProperty(new CG.prop('title', new CG.str()))
.addDataModelBinding(CG.common('IDataModelBindingsSimple'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vil si at vi må inkludere støtte for ekstern datamodell i v1 for å sørge for at det blir et helhetlig config-design 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vil merge den først siden da kan DIBK ta den i bruk, hva er galt med å legge til støtte for det senere?

Copy link
Contributor

@olemartinorg olemartinorg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit skeptical of naming this component Table. While it currently has support for repeating structures, it is not clear how we should expand this configuration to include support for non-repeating structures (like the current Grid and List components). If we don't land on something now, we risk having to integrating these concepts with a sub-optimal configuration later on, because the Table component name has already been reserved at that point. I noticed that my config suggestion received few comments last round, and I don't see any traces of inspiration from there in this PR.

"need to have", then prefix the description.

- B1: UI components should only receive data to display, and notify the app when data is changed.
- B2: UI components should live in a separate folder from the app itself, and have no dependencies to the app.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally i'm not sold on this one. I find it confusing when, for example, the table component here is mostly located 'elsewhere' and not in the folder where the layout component is. We've been working on doing the opposite for a while; moving code related to each component or feature to the folder named after that component or feature.

The counter-point is that these pure components are meant for sharing, but when does actually sharing work? I haven't seen any larger drives to integrate these things (docs, Studio, storybook) with our components, and without having that discussion this shareability might just be a shot in the dark, and one more layer of indirection that might just make development harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is like saying its confusing to use designsystemet as a view library.

The benefits of separating view components from the app logic includes but is not limited to:

1. Code Reusability: Pure view components are reusable across different parts of the app, as they are only concerned with how the UI looks, not how it behaves. This makes it easy to use the same component in multiple places without duplicating logic.

This is a more important point than reusing components with Studio or Storybook, but it also gives us that possibility.

2. Simplified Testing: With view components separated, you can test them independently without worrying about application logic. This isolation makes unit tests more focused and reliable, as you only need to check if a component renders correctly for given inputs (props) and not the entire app flow.

This is a huge issue in our app currently. This will ensure we can both move more testing to components which will be faster now and scale better in the future.

3. Easier Maintenance: Separation makes it easier to locate and fix bugs since you know exactly where the UI ends and the logic begins. This modularity also makes refactoring or redesigning the UI simpler, as you’re only changing visual components without touching business logic.

As we are reusing things like lists, tables, inputs etc, we only update in one spot as well.

4. Improved Readability: Clean separation between views and logic helps other developers (or your future self) understand the structure of the app more quickly.

They can identify what each part does at a glance, as there is a clear boundary between the UI layer and the functionality layer.

5. Scalability: As applications grow, so does their complexity. A separation between UI and logic allows for better scalability by organizing code into distinct concerns. This allows teams to work on specific parts of the application more independently, making collaboration smoother.

In the future it could make sense to have one team focused on input components, and another focused on the app logic.

Easier State Management: When the view components are separate, managing state becomes clearer. Logic components handle state, business rules, and data fetching, while view components remain “dumb” and only render the provided data. This results in a more predictable data flow.

A list of decision drivers. These are points which can differ in importance. If a point is "nice to have" rather than
"need to have", then prefix the description.

- B1: UI components should only receive data to display, and notify the app when data is changed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A relevant case to study here would be the bugfix in #2612. That fix is straight-forward, simple and easy to understand. With the suggested approach of only passing props for what to display, we'd instead have to:

  1. Import the hook, pass the result to the pure component (simple enough)
  2. Find usages for the pure component to figure out if we can make the new prop required or not. This might mean we'd have to look into how Studio, docs, and storybook uses the pure component, and plan for an upgrade path in case these parts are deployed non-atomically along with our package(s).
    • Or just never introduce required props, which forces us to deal with how to tackle undefined everywhere
  3. Deal with all the re-rendering of the parent, and hope it never reaches a critical level we cannot accept. For instance, if we pull in a new hook and that causes re-rendering all the time, did it really have to re-render? A component can be pure even though it has branches, and if only a rare selection of branches need the result of that hook, we'll re-render regardless with this approach.


- B1: UI components should only receive data to display, and notify the app when data is changed.
- B2: UI components should live in a separate folder from the app itself, and have no dependencies to the app.
- B3: UI components should live in a lib separately to the src folder to have stricter control of dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be part of the ADR when we also have stated that adding yarn workspaces or similar requires more research and testing? This should probably be pulled out and made into another ADR. I'm not entirely sure about the background for this to be a decision driver either - is it to enable sharing these pure components with other packages? In that case, I think a dedicated ADR is in order that also describes why that is needed, and why we couldn't go with other alternatives (why can't these other packages just use the design system as well -- why did this package have become a defacto design system?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, maybe it's my misunderstanding of ADRs. This is just documenting everything you talked about, even though the final solution fell short of supporting B3? Still, I didn't immediately see the connection to the problem context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the ADR conculdes that we should not introduce yarn workspaces or NX at this point, that would be separate ADR if we decied to investigate further. So that would as you say be pulled into a separate ADR.

But youre right, it could make sense that this ADR asked "should we introduce a component library?". However this was really already agreed upon in frontend architecture forum, so this ADR basically formulates how what was already agreed upon should be implemented.

Comment on lines +1 to +11
## IMPORTANT

Components in this folder should be 'dumb', meaning the should have no knowledge about the containing app.

They should implement three things:

1. Receive and display data in the form of props.
2. Report data changes in a callback.
3. Receive and display the validity of the data.

These components will form the basis of our future component library, and will enable refactoring of the frontend app.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that's how it's meant to be used, but might as well link to the ADR from here as well. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more a general guideline for the components in the folder. My preference would be to have this lib completely separate from the src folder so that importing the wrong way is not possible, but as we have not decided to do this yet, this readme serves as a litt reminder :)

Comment on lines +85 to +136
{data.map((rowData, rowIndex) => (
<Table.Row key={rowIndex}>
{columns.map((col, colIndex) => {
const cellValues = col.accessors
.filter((accessor) => !!pick(accessor, rowData))
.map((accessor) => pick(accessor, rowData));
if (cellValues.every((value) => value == null)) {
return (
<Table.Cell
key={colIndex}
data-header-title={col.header}
>
-
</Table.Cell>
);
}

if (col.renderCell) {
return (
<Table.Cell
key={colIndex}
data-header-title={col.header}
>
{col.renderCell(cellValues, rowData)}
</Table.Cell>
);
}

if (cellValues.length === 1) {
return (
<Table.Cell
data-header-title={col.header}
key={colIndex}
>
{cellValues.map(formatIfDate)}
</Table.Cell>
);
}

return (
<Table.Cell
key={colIndex}
data-header-title={col.header}
>
<ul>
{cellValues.map(formatIfDate).map((value, idx) => (
<li key={idx}>{value}</li>
))}
</ul>
</Table.Cell>
);
})}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a matter of taste, but I would prefer to extract large chunks like this into one or more separate components.

src/layout/Table/config.ts Outdated Show resolved Hide resolved
src/layout/Table/config.ts Outdated Show resolved Hide resolved
src/layout/Table/config.ts Outdated Show resolved Hide resolved
src/layout/Table/index.tsx Show resolved Hide resolved
src/layout/Table/typeguards.ts Outdated Show resolved Hide resolved
@adamhaeger
Copy link
Contributor Author

I'm a bit skeptical of naming this component Table. While it currently has support for repeating structures, it is not clear how we should expand this configuration to include support for non-repeating structures (like the current Grid and List components). If we don't land on something now, we risk having to integrating these concepts with a sub-optimal configuration later on, because the Table component name has already been reserved at that point. I noticed that my config suggestion received few comments last round, and I don't see any traces of inspiration from there in this PR.

I had not seen your comment there, Ill have a look :)


const { formData } = useDataModelBindings(dataModelBindings, 1, 'raw');
const { title } = textResourceBindings ?? {};
const { langAsString } = useLanguage();

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable langAsString.
const { formData } = useDataModelBindings(item.dataModelBindings, 1, 'raw');
const removeFromList = FD.useRemoveFromListCallback();
const { title, description, help } = item.textResourceBindings ?? {};
const { langAsString } = useLanguage();

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable langAsString.
Copy link

sonarcloud bot commented Oct 28, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
48.2% Coverage on New Code (required ≥ 65%)
33.33% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarCloud

@adamhaeger
Copy link
Contributor Author

I'm a bit skeptical of naming this component Table. While it currently has support for repeating structures, it is not clear how we should expand this configuration to include support for non-repeating structures (like the current Grid and List components). If we don't land on something now, we risk having to integrating these concepts with a sub-optimal configuration later on, because the Table component name has already been reserved at that point. I noticed that my config suggestion received few comments last round, and I don't see any traces of inspiration from there in this PR.

My thoughts: The table component should only support tabular data.

In case of grid, it is more of a layout compoment and should remain as such.

In case of List, it actually does show tabular data, so it is a candidate for being supported in table.

In any case, when we have app-components, the table itself can be reused in other layout components so that we have one way to show and style a table in our application.

Risk of Future Integration Issues: Without deciding on an approach now, there’s a risk of future integration challenges if the name "Table" is already in use.

There is always the chance that we get the config wrong the first time around.
We will have to deprecate components many times as we learn, case in point Summary2.
That said I believe the suggested config is very minimal, flexible and easy to expand, so ut should be straight forward to adapt it to external data, and option list.

4. Lack of Feedback: Few comments were received on a previous configuration suggestion, and there’s no indication that it inspired changes in the current PR.
I had not seen the comment, have looked at it now and Id say its quite similar, other than the possibility to define static tables, which in my view should be done with grid like stated above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/product-feature Pull requests containing new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Table component
8 participants