-
Notifications
You must be signed in to change notification settings - Fork 31
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
Draft
adamhaeger
wants to merge
32
commits into
main
Choose a base branch
from
feature/table
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Feature/table #2593
Changes from 28 commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
8101e4d
wip: made basic table component
adamhaeger 5b43cbc
wip
adamhaeger 4bc3a0d
Merge branch 'main' into feature/table
adamhaeger be190fc
wip
adamhaeger 8d32b21
first working example of new table setup
adamhaeger 0b5de14
clean
adamhaeger a7ed412
basic add to form working
adamhaeger 7258547
rendering table in summary
adamhaeger 60f5207
switched to simplebinding for table
adamhaeger a773367
add to list working boys
adamhaeger 1916114
delete woring
adamhaeger b1db8ad
edit and delete working
adamhaeger 5fbb6cb
wip
adamhaeger 49ea97a
support multiple accessors
adamhaeger 9cb7e12
wip
adamhaeger 6fadfc3
wip
adamhaeger 7d09df1
temp looser type checking until we find a better solution
adamhaeger 3236593
ADR update
adamhaeger 384a75b
add mobile table styling
Magnusrm 1f30861
added test, updated ADR
adamhaeger 5b724cd
Merge branch 'feature/table' of https://github.com/Altinn/app-fronten…
adamhaeger e66ab7a
cleanup
adamhaeger 3bc7897
fix button cell styling
Magnusrm 604f811
fix mobile styling
Magnusrm 37c6900
add accessible header for action buttons
Magnusrm 7ba8796
merge
adamhaeger 401f770
change to list when showing multiple values in the same cell
adamhaeger 36905da
Merge branch 'main' into feature/table
adamhaeger 76d4cef
move caption component and add to Table component
Magnusrm d71d2d0
fix mobile caption
Magnusrm 2d51bc5
Update adr/001-component-library.md
adamhaeger 75b931f
Added support for nested accessors in the data table
adamhaeger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,60 @@ | ||||||
# Introduce component library | ||||||
|
||||||
- Status: Proposed | ||||||
- 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. | ||||||
- Makes refactoring the app framework a lot harder. | ||||||
- Leads to unclear interfaces between UI components and the app framework. | ||||||
- Makes developing UI components complex without deep understanding of the application. | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
? |
||||||
## 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, notify the app when data is changed, and notify validity of user input. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I suggest that we remove the validation of user input part, as it makes little sense to leave this to UI components in our context |
||||||
- 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. | ||||||
|
||||||
## 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. | ||||||
|
||||||
### A2 | ||||||
adamhaeger marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
- 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. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. Report if the input is valid or not. | ||
|
||
These components will form the basis of our future component library, and will enable refactoring of the frontend app. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
.table { | ||
width: 100%; | ||
} | ||
|
||
.buttonContainer { | ||
display: flex; | ||
justify-content: end; | ||
gap: var(--fds-spacing-2); | ||
} | ||
|
||
.mobileTable { | ||
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; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,191 @@ | ||
// 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(); | ||
}); | ||
|
||
test('non-string date values are converted to string', () => { | ||
const dataWithNumberDate = [{ id: 1, name: 'Alice', date: 1234567890, amount: 100 }]; | ||
render( | ||
<AppTable | ||
data={dataWithNumberDate} | ||
columns={columns} | ||
/>, | ||
); | ||
expect(screen.getByText('1234567890')).toBeInTheDocument(); | ||
}); | ||
}); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have linked examples of code/components for some of these points, I'm sure you've discussed and looked at tons of examples here to inform this assertion but for myself presently and future readers it would be helpful to know the context here 😄