Feature/table #2593
Feature/table #2593
Conversation
…d-react into feature/table
Co-authored-by: Magnus Revheim Martinsen <mrmartinsen.96@gmail.com>
src/layout/Table/config.ts
Outdated
| .extends(CG.common('LabeledComponentProps')) | ||
| .extendTextResources(CG.common('TRBLabel')) | ||
| .addProperty(new CG.prop('title', new CG.str())) | ||
| .addDataModelBinding(CG.common('IDataModelBindingsSimple')) |
There was a problem hiding this comment.
Vil si at vi må inkludere støtte for ekstern datamodell i v1 for å sørge for at det blir et helhetlig config-design 😄
There was a problem hiding this comment.
Vil merge den først siden da kan DIBK ta den i bruk, hva er galt med å legge til støtte for det senere?
Co-authored-by: Camilla Marie Dalan <camillamdalan@gmail.com>
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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:
- Import the hook, pass the result to the pure component (simple enough)
- 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
undefinedeverywhere
- Or just never introduce required props, which forces us to deal with how to tackle
- 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. |
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| ## 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. |
There was a problem hiding this comment.
Not sure that's how it's meant to be used, but might as well link to the ADR from here as well. 🙏
There was a problem hiding this comment.
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 :)
| {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> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
Might be a matter of taste, but I would prefer to extract large chunks like this into one or more separate components.
I had not seen your comment there, Ill have a look :) |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| const { formData } = useDataModelBindings(dataModelBindings, 1, 'raw'); | ||
| const { title } = textResourceBindings ?? {}; | ||
| const { langAsString } = useLanguage(); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
| 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
|
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. 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. |
Description
Related Issue(s)
Verification/QA
kind/*label to this PR for proper release notes grouping