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
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
8101e4d
wip: made basic table component
adamhaeger Oct 9, 2024
5b43cbc
wip
adamhaeger Oct 11, 2024
4bc3a0d
Merge branch 'main' into feature/table
adamhaeger Oct 11, 2024
be190fc
wip
adamhaeger Oct 11, 2024
8d32b21
first working example of new table setup
adamhaeger Oct 11, 2024
0b5de14
clean
adamhaeger Oct 11, 2024
a7ed412
basic add to form working
adamhaeger Oct 14, 2024
7258547
rendering table in summary
adamhaeger Oct 15, 2024
60f5207
switched to simplebinding for table
adamhaeger Oct 15, 2024
a773367
add to list working boys
adamhaeger Oct 15, 2024
1916114
delete woring
adamhaeger Oct 15, 2024
b1db8ad
edit and delete working
adamhaeger Oct 16, 2024
5fbb6cb
wip
adamhaeger Oct 16, 2024
49ea97a
support multiple accessors
adamhaeger Oct 17, 2024
9cb7e12
wip
adamhaeger Oct 17, 2024
6fadfc3
wip
adamhaeger Oct 17, 2024
7d09df1
temp looser type checking until we find a better solution
adamhaeger Oct 17, 2024
3236593
ADR update
adamhaeger Oct 17, 2024
384a75b
add mobile table styling
Magnusrm Oct 17, 2024
1f30861
added test, updated ADR
adamhaeger Oct 17, 2024
5b724cd
Merge branch 'feature/table' of https://github.com/Altinn/app-fronten…
adamhaeger Oct 17, 2024
e66ab7a
cleanup
adamhaeger Oct 17, 2024
3bc7897
fix button cell styling
Magnusrm Oct 17, 2024
604f811
fix mobile styling
Magnusrm Oct 17, 2024
37c6900
add accessible header for action buttons
Magnusrm Oct 17, 2024
7ba8796
merge
adamhaeger Oct 17, 2024
401f770
change to list when showing multiple values in the same cell
adamhaeger Oct 17, 2024
36905da
Merge branch 'main' into feature/table
adamhaeger Oct 17, 2024
76d4cef
move caption component and add to Table component
Magnusrm Oct 17, 2024
d71d2d0
fix mobile caption
Magnusrm Oct 17, 2024
2d51bc5
Update adr/001-component-library.md
adamhaeger Oct 17, 2024
75b931f
Added support for nested accessors in the data table
adamhaeger Oct 18, 2024
b698b60
wip
adamhaeger Oct 21, 2024
7be3f07
Merge branch 'main' into feature/table
adamhaeger Oct 22, 2024
8b17217
Merge branch 'main' into feature/table
adamhaeger Oct 22, 2024
0c6e825
Merge branch 'main' into feature/table
adamhaeger Oct 23, 2024
558db33
Merge branch 'main' into feature/table
adamhaeger Oct 23, 2024
6381270
Added size support
adamhaeger Oct 23, 2024
7149f01
Update adr/001-component-library.md
adamhaeger Oct 23, 2024
1b57b25
Update adr/001-component-library.md
adamhaeger Oct 23, 2024
2b11772
Added support for zebra, updated ADR
adamhaeger Oct 23, 2024
70b95aa
ADR update
adamhaeger Oct 23, 2024
e68c20a
merge
adamhaeger Oct 23, 2024
b510f56
cleanup
adamhaeger Oct 23, 2024
d1d9532
Added support for delete row
adamhaeger Oct 23, 2024
50b2ca4
Merge branch 'main' into feature/table
adamhaeger Oct 24, 2024
b02740c
cleanup and wcag update
adamhaeger Oct 24, 2024
c658190
cleanup
adamhaeger Oct 24, 2024
066bd16
Update src/features/formData/FormDataWriteStateMachine.tsx
adamhaeger Oct 24, 2024
3fed2fa
pass caption to table as parameter
Magnusrm Oct 25, 2024
514a638
added displayValue test for table
adamhaeger Oct 28, 2024
82d5d6b
got rid of cast
adamhaeger Oct 28, 2024
3906055
clean
adamhaeger Oct 28, 2024
3219de1
Merge branch 'main' into feature/table
adamhaeger Oct 28, 2024
0738028
Update src/app-components/table/Table.tsx
adamhaeger Oct 28, 2024
5a97704
clean
adamhaeger Oct 28, 2024
3d566f4
Merge branch 'feature/table' of https://github.com/Altinn/app-fronten…
adamhaeger Oct 28, 2024
047fa3f
Merge branch 'main' into feature/table
adamhaeger Oct 28, 2024
7d2aa58
updates after code review
adamhaeger Oct 28, 2024
a8766e9
updates after code review
adamhaeger Oct 28, 2024
69d83b2
Added some datamodel validation
adamhaeger Oct 28, 2024
1294d9d
Added some datamodel validation
adamhaeger Oct 28, 2024
4828bf3
clean
adamhaeger Oct 28, 2024
1db187d
using selecter in useNodeItem
adamhaeger Oct 28, 2024
39219a5
clean
adamhaeger Oct 28, 2024
d6f1a27
clean
adamhaeger Oct 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions adr/001-component-library.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Introduce component library

- Status: Accepted
- Deciders: Team
- Date: 17.10.2024

## Result

A1: Component library is introduced, firstly as just a folder in our current setup, adding yarn workspaces or similar requires more research and testing

## Problem context

Today our UI components are tightly coupled to the app in which they are rendered.

This leads to several issues:

- Makes it hard to do component testing outside a fully rendered app.
- See tests in `test/e2e/integration` for examples of how we have most of our tests in integration tests because components need to be running inside an app to work.
- See `src/test/renderWithProviders.tsx` for examples of how much scaffolding we need to do to run component tests currently. This also leads to very slow component tests.
- Makes refactoring the app framework a lot harder.
- If we search for `useNodeItem` in src/layout we get over 100 hits. If we make changes or remove hooks like this, every single UI component must be updated.
- Leads to unclear interfaces between UI components and the app framework.
adamhaeger marked this conversation as resolved.
Show resolved Hide resolved
- See: `src/layout/Button/ButtonComponent.tsx`. This component references `node.parent` which confuses the role and responsibility of the button component.
- Makes developing UI components complex without deep understanding of the application.
- Enables sharing of pure components (docs, Studio, storybook)

adamhaeger marked this conversation as resolved.
Show resolved Hide resolved
## Decision drivers

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.

- 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.

- 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.


## Alternatives considered

List the alternatives that were considered as a solution to the problem context.

- A1: Simply put a new folder inside the src folder.
- A2: Use yarn workspaces to manage the library separately from the src folder.
- A3: Set up NX.js to manage our app and libraries.

## Pros and cons

List the pros and cons with the alternatives. This should be in regards to the decision drivers.

### A1

- Good because B1 and B2 is covered
- Allows us to really quickly get started with a component library
- Bad, because it does not fulfill B3. If we simply use a new folder, it will be up to developers to enforce the rules of the UI components, like the avoiding dependencies to the app.

### A2

- Good, because this alternative adheres to B1, B2 and B3.
- This way our libs would live separately to the app, and it would be obvious that it is a lib.
- The con is that it takes more setup.

### A3

- Good, because this alternative adheres to B1, B2 and B3.
- This way our libs would live separately to the app, and it would be obvious that it is a lib.
- Also gives us powerful monorepo tooling.
- Bad because it takes a lot more time to set up, and might be overkill before we have decided to integrate frontend and backend into monorepo.
11 changes: 11 additions & 0 deletions src/app-components/readme.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,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.
Comment on lines +1 to +11
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 :)

88 changes: 88 additions & 0 deletions src/app-components/table/Table.module.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
.table {
width: 100%;
}

.buttonContainer {
display: flex;
justify-content: end;
gap: var(--fds-spacing-2);
}

.mobileTable {
display: block;
}

.mobileTable caption {
display: block;
}

.mobileTable thead {
display: none;
}

.mobileTable th {
display: block;
border: none;
}

.mobileTable tbody,
.mobileTable tr {
display: block;
}

.mobileTable td {
display: flex;
flex-direction: column;
border: none;
padding: var(--fds-spacing-2) 0;
}

.mobileTable td .contentWrapper {
display: flex;
flex-direction: row;
align-items: center;
width: 100%;
}

.mobileTable tbody tr:last-child {
border-bottom: 2px solid var(--fds-semantic-border-neutral-default);
}

.mobileTable tbody tr:first-child {
border-top: 2px solid var(--fds-semantic-border-neutral-default);
}

.mobileTable tr {
border-bottom: 1px solid var(--fds-semantic-border-neutral-default);
padding-top: var(--fds-spacing-3);
padding-bottom: var(--fds-spacing-3);
}

.mobileTable tr:hover td {
background-color: unset;
}

.mobileTable td[data-header-title]:not([data-header-title=''])::before,
.mobileTable th[data-header-title]:not([data-header-title=''])::before {
content: attr(data-header-title);
display: block;
text-align: left;
font-weight: 500;
}

.mobileTable td .buttonContainer {
justify-content: start;
}

.visuallyHidden {
border: none;
padding: 0;
margin: 0;
position: absolute;
height: 1px;
width: 1px;
overflow: hidden;
clip: rect(1px 1px 1px 1px);
clip-path: inset(50%);
white-space: nowrap;
}
180 changes: 180 additions & 0 deletions src/app-components/table/Table.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
// AppTable.test.tsx
import React from 'react';

import { fireEvent, render, screen } from '@testing-library/react';

import { AppTable } from 'src/app-components/table/Table';

// Sample data
const data = [
{ id: 1, name: 'Alice', date: '2023-10-05', amount: 100 },
{ id: 2, name: 'Bob', date: '2023-10-06', amount: 200 },
];

// Columns configuration
const columns = [
{ header: 'Name', accessors: ['name'] },
{ header: 'Date', accessors: ['date'] },
{ header: 'Amount', accessors: ['amount'] },
];

// Action buttons configuration
const actionButtons = [
{
onClick: jest.fn(),
buttonText: 'Edit',
icon: null,
},
{
onClick: jest.fn(),
buttonText: 'Delete',
icon: null,
},
];

describe('AppTable Component', () => {
test('renders table with correct headers', () => {
render(
<AppTable
data={data}
columns={columns}
/>,
);
expect(screen.getByText('Name')).toBeInTheDocument();
expect(screen.getByText('Date')).toBeInTheDocument();
expect(screen.getByText('Amount')).toBeInTheDocument();
});

test('renders correct number of rows', () => {
render(
<AppTable
data={data}
columns={columns}
/>,
);
const rows = screen.getAllByRole('row');
expect(rows.length).toBe(data.length + 1); // +1 for header row
});

test('renders action buttons when provided', () => {
render(
<AppTable
data={data}
columns={columns}
actionButtons={actionButtons}
/>,
);
expect(screen.getAllByText('Edit').length).toBe(data.length);
expect(screen.getAllByText('Delete').length).toBe(data.length);
});

test('correctly formats dates in cells', () => {
render(
<AppTable
data={data}
columns={columns}
/>,
);
expect(screen.getByText('05.10.2023')).toBeInTheDocument();
expect(screen.getByText('06.10.2023')).toBeInTheDocument();
});

test('uses renderCell function when provided', () => {
const columnsWithRenderCell = [
...columns,
{
header: 'Custom',
accessors: ['name', 'amount'],
renderCell: (values) => `Name: ${values[0]}, Amount: ${values[1]}`,
},
];
render(
<AppTable
data={data}
columns={columnsWithRenderCell}
/>,
);
expect(screen.getByText('Name: Alice, Amount: 100')).toBeInTheDocument();
expect(screen.getByText('Name: Bob, Amount: 200')).toBeInTheDocument();
});

test('calls action button onClick when clicked', () => {
const onClickMock = jest.fn();
const actionButtonsMock = [
{
onClick: onClickMock,
buttonText: 'Edit',
icon: null,
},
];

render(
<AppTable
data={data}
columns={columns}
actionButtons={actionButtonsMock}
/>,
);

const editButtons = screen.getAllByText('Edit');
fireEvent.click(editButtons[0]);
expect(onClickMock).toHaveBeenCalledWith(0, data[0]);

fireEvent.click(editButtons[1]);
expect(onClickMock).toHaveBeenCalledWith(1, data[1]);

expect(onClickMock).toHaveBeenCalledTimes(2);
});

test('renders "-" when cell values are null or undefined', () => {
const dataWithNull = [
{ id: 1, name: 'Alice', date: null, amount: 100 },
{ id: 2, name: 'Bob', date: '2023-10-06', amount: 200 },
];
render(
<AppTable
data={dataWithNull}
columns={columns}
/>,
);
expect(screen.getAllByText('-').length).toBe(1);
});

test('does not render action buttons column when actionButtons is not provided', () => {
render(
<AppTable
data={data}
columns={columns}
/>,
);
const headerCells = screen.getAllByRole('columnheader');
expect(headerCells.length).toBe(columns.length);
});

test('renders extra header cell when actionButtons are provided', () => {
render(
<AppTable
data={data}
columns={columns}
actionButtons={actionButtons}
/>,
);
const headerCells = screen.getAllByRole('columnheader');
expect(headerCells.length).toBe(columns.length + 1);
});

test('non-date values are not changed by formatIfDate', () => {
const dataWithNonDate = [
{ id: 1, name: 'Alice', date: 'Not a date', amount: 100 },
{ id: 2, name: 'Bob', date: 'Also not a date', amount: 200 },
];
render(
<AppTable
data={dataWithNonDate}
columns={columns}
/>,
);
expect(screen.getByText('Not a date')).toBeInTheDocument();
expect(screen.getByText('Also not a date')).toBeInTheDocument();
});
});
Loading
Loading