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

feat: Add table component to viewer #920

Merged
merged 15 commits into from
Dec 6, 2023
Merged

Conversation

vsgoulart
Copy link
Contributor

I was having some issues with the tests, I'll add them tomorrow morning

Closes #892

  • This PR adds a new form-js element or visually changes an existing component.

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Nov 29, 2023
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer November 30, 2023 04:06 Destroyed
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer November 30, 2023 08:26 Destroyed
@Skaiir
Copy link
Contributor

Skaiir commented Nov 30, 2023

Before I start looking into the code, just a few first impressions / remarks on my side. I checked the parent issue, after this PR there's carbonization but I guess you're still planning to change non-carbon styles as well a bit? Anyways there's a few style issues but I won't discuss them for now assuming the above.


Adding tables pollutes the input data:

grafik

This section shouldn't be programmatically updated as a side effect of adding a component. I guess this is probably related to the way we add the sample data. I also have some reservations about that but we'll discuss on the full PR.


Pagination by default is quite a lot right out of the gates:

grafik

Very visually complex, do we really need to introduce pagination immediately to our users? Especially with a height of 1 which makes it:

  • Not a reasonable default, meaning it's a property everyone will have to modify to get a "standard" behavior
  • Not the default you usually get from toggling pagination, leading to these kinds of interactions where toggling the checkbox actually changes things

firefox_Gvn7dEDJZx

The that second point, I think we could also just keep the page size in the schema, up for debate.


Keys and data-sources should probably not be overlappable

grafik

Binding a key to the same path as the dataSource destroys the data and hence leads to the table showing nothing. This is normal sanitation behavior though. So at the very least they should be exclusive.


Data-sources bypass groups paths

grafik

This is kind of an interesting one. Because the way we've gone about this is, text path definitions stack with eachother, but feel expressions require using of 'this' keyword to access the local scope (they default to the root). Should we copy the model here? I'm not sure.


Drag and drop is compressed on small screens

The minimum width makes things difficult when it comes to having two items side by side:

grafik

@pinussilvestrus
Copy link
Contributor

pinussilvestrus commented Nov 30, 2023

This section shouldn't be programmatically updated as a side effect of adding a component. I guess this is probably related to the way we add the sample data. I also have some reservations about that but we'll discuss on the full PR.

I think the demo data makes sense with the rationale provided via #905. What makes it a bit clumsy is the fact you will likely update the dataSource after first create, which needs another update in the data editor everytime.

@vsgoulart
Copy link
Contributor Author

I also have some reservations about that but we'll discuss on the full PR.

@Skaiir I split this task into smaller PRs, everything is merged into 888-table-component, so this is the last big PR we'll have for epic. After the canonization we can just merge the feature branch into develop since everything is already reviewed

but I guess you're still planning to change non-carbon styles as well a bit? Anyways there's a few style issues but I won't discuss them for now assuming the above.

Yeah, I'll create a separate last PR for that

This section shouldn't be programmatically updated as a side effect of adding a component. I guess this is probably related to the way we add the sample data.

This was agreed on the define phase. As @pinussilvestrus mentioned the goal is to provide some data to render so the user isn't completely lost when adding a new table into the schema, this will probably be immediately edited out when modelling it and I don't believe users will add too many tables too. Anyway, it's too late to not go with it now, we have iterate over it when we have user feedback.

Very visually complex, do we really need to introduce pagination immediately to our users? Especially with a height of 1 which makes it:
Not a reasonable default, meaning it's a property everyone will have to modify to get a "standard" behavior
Not the default you usually get from toggling pagination, leading to these kinds of interactions where toggling the checkbox actually changes things

The default is 10 rows (it was 1 originally because it was the value I was using for testing). The pagination buttons will also only be visible if there's more than 1 page.
Having pagination is basically the whole purpose of having the table component, because otherwise the form would be giant and the user would have to scroll all the way down to complete the form. (Another option would be limiting the height and having a vertical scroll, but we decided this is a bad UX because we would have a scroll area inside of a scroll area)

The other points are bugs, I'll fix them tomorrow

@vsgoulart vsgoulart removed the request for review from pinussilvestrus November 30, 2023 18:00
@Skaiir
Copy link
Contributor

Skaiir commented Dec 1, 2023

The default is 10 rows (it was 1 originally because it was the value I was using for testing). The pagination buttons will also only be visible if there's more than 1 page.

Yeah this was also a bug right? That's fine, we can have the table paginated by default I just mean we don't need to visually show them pagination in action the moment they open the table. But I see you fixed that.


This was agreed on the define phase. As @pinussilvestrus mentioned the goal is to provide some data to render so the user isn't completely lost when adding a new table into the schema, this will probably be immediately edited out when modelling it and I don't believe users will add too many tables too. Anyway, it's too late to not go with it now, we have iterate over it when we have user feedback.

On the basis that we don't have a better alternative right now, we can do this. I mean we'll most likely overhaul input and output data pretty soon so it's not going to be a problem for too long. Maybe we'll have requests for statically defined tables (who knows) and then we can use that for the defaulting.

In general though, I dislike this because it's a unique behavior to tables, it goes against "no surprises" entirely, and while convenient maybe for the first 1 or 2 times you add a table, becomes visual noise the moment you understand how they work. I'm not against generating some sample data but the way I see it, it should be an extra action. But yeah let's wait on feedback.


Another thing that's a bit inconsistent, the values displayed in the editor aren't matching those of the viewer, which is new:

grafik

Usually we have no values at all, but this is also not great:

grafik

I think we need to agree on some visual language practices for elements which are populated through a source. Something like the skeletons in carbon would work well I think. We can take this on as a follow-up. @volodymyr-melnykc If you have any opinions there, feel free to chime in.

@vsgoulart vsgoulart force-pushed the 888-table-component branch from ec33b55 to a3bdd2d Compare December 1, 2023 10:33
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 1, 2023 10:46 Destroyed
@volodymyr-melnykc
Copy link

volodymyr-melnykc commented Dec 1, 2023

Another thing that's a bit inconsistent, the values displayed in the editor aren't matching those of the viewer, which is new:

@Skaiir @vsgoulart I see what's the issue. What we can do is show generic content for a table in the Form Definition, even for the sample data.
Meaning instead of showing columns "Id", "Name" and "Address", have generic "Column 1", "Column 2", "Column 3".
The actual table content is to be shown only in the Form Preview.

image

Usually we have no values at all, but this is also not great:

I think we need to agree on some visual language practices for elements which are populated through a source. Something like the skeletons in carbon would work well I think.

In the long term, it would be really good to merge these 2 views (Form Definition and Form Preview) into one, from where the user can edit and preview a form at the same time. We also got several user requests for this.

@vsgoulart vsgoulart force-pushed the 888-table-component branch from a3bdd2d to 4cdc290 Compare December 4, 2023 09:53
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 4, 2023 09:57 Destroyed
@volodymyr-melnykc
Copy link

volodymyr-melnykc commented Dec 4, 2023

@vsgoulart @Skaiir After testing the feature for a bit longer, I'd suggest a few improvements for table columns and rows preview on the canvas.

  • If a table header source is static (the type is List of items), the header columns in both Form Definition and Form Preview canvases should reflect the user's input. Keeping the table headers up-to-date in the Form Definition canvas helps the user relate to what the table is about.
    • Currently, when changing a header name from the Properties panel, it's reflected in the Form Preview (correct) but not in the Form Definition (not correct).
image

--

  • If a table header source is dynamic - coming from an expression, then show generic column header names in the Form Definition canvas:
image

--

  • Since the table content comes from variables, previewing the content in rows in the Form Definition canvas can always be generic, even for the sample data. E.g.:
image

It's also related to the issue that @Skaiir brought up earlier:

Another thing that's a bit inconsistent, the values displayed in the editor aren't matching those of the viewer, which is new:

cc @christian-konrad

@vsgoulart vsgoulart force-pushed the 888-table-component branch from 4cdc290 to 686957a Compare December 4, 2023 12:14
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 4, 2023 12:14 Destroyed
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 4, 2023 13:30 Destroyed
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 4, 2023 16:08 Destroyed
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 4, 2023 16:11 Destroyed
@Skaiir
Copy link
Contributor

Skaiir commented Dec 4, 2023

In the long term, it would be really good to merge these 2 views (Form Definition and Form Preview) into one, from where the user can edit and preview a form at the same time. We also got several user requests for this.

Well they aren't the same views so we can't exactly merge them, however something pretty close to that would be to have a single panel with an easily accessible toggle (both via keyboard and UI) between edit mode and view mode. This is pretty common in a lot of modeling tools.

So it's less about changing how things work and more about just shifting the UI around to have more space to work with. The only thing that is unclear there is where the input/output data should be going, but we can have some chats and wireframes on this.

@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 5, 2023 11:03 Destroyed
Copy link
Contributor

@Skaiir Skaiir left a comment

Choose a reason for hiding this comment

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

Few review comments, nothing major though, nice work.

@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 5, 2023 16:02 Destroyed
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 5, 2023 16:04 Destroyed
@vsgoulart vsgoulart requested a review from Skaiir December 5, 2023 16:04
@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 5, 2023 16:09 Destroyed
@volodymyr-melnykc
Copy link

volodymyr-melnykc commented Dec 5, 2023

@vsgoulart The latest update looks great! 🚀

I have a minor comment regarding the visual layout.
Can we add vertical spacing between the label and table? Currently, it's 0, to be 4px (e.g. via "row-gap").

image

@github-actions github-actions bot temporarily deployed to demo-892-table-viewer December 5, 2023 16:42 Destroyed
@vsgoulart
Copy link
Contributor Author

@volodymyr-melnykc done

@Skaiir
Copy link
Contributor

Skaiir commented Dec 6, 2023

Nice work :)

@vsgoulart vsgoulart merged commit 3b114f2 into 888-table-component Dec 6, 2023
11 of 13 checks passed
@vsgoulart vsgoulart deleted the 892-table-viewer branch December 6, 2023 13:48
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 6, 2023
vsgoulart added a commit that referenced this pull request Dec 8, 2023
* feat: Add table component to viewer

* fix: Fix default rowCount

* feat: Add generic data on editor view

* fix: Refactor form field table styles

* test: Add tests

* fix: Turn data source into FEEL only

* chore: Remove unnecessary new line

* chore: Remove FEEL check on dataSource

* chore: Sort by asc first

* chore: Remove unnecessary label check

* chore: Use const instead of let

* fix: Create EditorTable

* chore: Fix formatting

* chore: Make label id optional

* fix: Add row gap
vsgoulart added a commit that referenced this pull request Dec 11, 2023
* feat: Add table component to viewer

* fix: Fix default rowCount

* feat: Add generic data on editor view

* fix: Refactor form field table styles

* test: Add tests

* fix: Turn data source into FEEL only

* chore: Remove unnecessary new line

* chore: Remove FEEL check on dataSource

* chore: Sort by asc first

* chore: Remove unnecessary label check

* chore: Use const instead of let

* fix: Create EditorTable

* chore: Fix formatting

* chore: Make label id optional

* fix: Add row gap
vsgoulart added a commit that referenced this pull request Dec 13, 2023
* feat: Add table component to viewer

* fix: Fix default rowCount

* feat: Add generic data on editor view

* fix: Refactor form field table styles

* test: Add tests

* fix: Turn data source into FEEL only

* chore: Remove unnecessary new line

* chore: Remove FEEL check on dataSource

* chore: Sort by asc first

* chore: Remove unnecessary label check

* chore: Use const instead of let

* fix: Create EditorTable

* chore: Fix formatting

* chore: Make label id optional

* fix: Add row gap
vsgoulart added a commit that referenced this pull request Dec 14, 2023
* chore: update dependency lerna to v8 (#914)

* chore: update dependency lerna to v8

* deps: remove legacy lerna scripts

Cf. https://lerna.js.org/docs/legacy-package-management

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Niklas Kiefer <[email protected]>
Co-authored-by: Vinícius Goulart <[email protected]>

* chore(CI): updated snapshots [skip ci]

* deps: update dependency didi to v10 (#909)

* deps: update dependency didi to v10

* chore: Bump to [email protected]

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Vinicius Goulart <[email protected]>

* chore(CI): updated snapshots [skip ci]

* chore: Bump to [email protected] (#927)

* deps: update dependency feelin to v3 (#941)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore: Bump `schemaVersion` to 13 (#896)

* chore: Bump schemaVersion to 13

* chore: Fix formatting

Co-authored-by: Niklas Kiefer <[email protected]>

* fix: Fix schema version range on docs

* chore: Remove newline

* feat: Make columns and columnsExpression mutually exclusive

* feat: Add custom error message

---------

Co-authored-by: Niklas Kiefer <[email protected]>

* chore: Fix typos in comments

* feat: Add table component to editor (#905)

* feat: Add table component to editor

* chore: Replace key with id

* chore: Fix typo

* feat: Add dataSource to schema

* fix: Rework initialDemoData and add test

* chore: Update formatting

Co-authored-by: Niklas Kiefer <[email protected]>

---------

Co-authored-by: Niklas Kiefer <[email protected]>

* feat: Add table entries to properties panel (#912)

* chore: Bump `schemaVersion` to 13 (#896)

* chore: Bump schemaVersion to 13

* chore: Fix formatting

Co-authored-by: Niklas Kiefer <[email protected]>

* fix: Fix schema version range on docs

* chore: Remove newline

* feat: Make columns and columnsExpression mutually exclusive

* feat: Add custom error message

---------

Co-authored-by: Niklas Kiefer <[email protected]>

* feat: Add table component to editor (#905)

* feat: Add table component to editor

* chore: Replace key with id

* chore: Fix typo

* feat: Add dataSource to schema

* fix: Rework initialDemoData and add test

* chore: Update formatting

Co-authored-by: Niklas Kiefer <[email protected]>

---------

Co-authored-by: Niklas Kiefer <[email protected]>

* chore: Remove commented code

* feat: Add table entries to properties panel

* fix: Fix tooltip

Co-authored-by: Niklas Kiefer <[email protected]>

* chore: Rename id

* feat: Use behavior to manage column headers

* chore: Fix test description

Co-authored-by: Niklas Kiefer <[email protected]>

---------

Co-authored-by: Niklas Kiefer <[email protected]>

* feat: Add table component to viewer (#920)

* feat: Add table component to viewer

* fix: Fix default rowCount

* feat: Add generic data on editor view

* fix: Refactor form field table styles

* test: Add tests

* fix: Turn data source into FEEL only

* chore: Remove unnecessary new line

* chore: Remove FEEL check on dataSource

* chore: Sort by asc first

* chore: Remove unnecessary label check

* chore: Use const instead of let

* fix: Create EditorTable

* chore: Fix formatting

* chore: Make label id optional

* fix: Add row gap

* fix: Add missing leftovers from table component (#938)

* feat: Add Carbon compatibility styles (#937)

* feat: Add Carbon compatibility styles

* test: Add Carbon tests

* feat: Add dynamic lists component (#808)

* chore: rename expression-language folder

* wip: implemented protoype repeatRenderManager

* feat: `subform` component

* feat: integrated prototype repeatRenderModule

* feat: hooked up indexing to update cycle

Related to #796

* feat: subform properties panel configuration

Related to #796

* feat: added group styles to subform

* feat: form initializer works with repeatable

Related to #796

* chore: display key and path from config

Related to #796

* feat: enforce path for repeatable groups

Related to #796

* feat: adjusted getSubmitData for repeatable

Related to #796

* feat: repeat render manager

Closes #796

* chore: renamed subform to dynamic list

Related to #796

* chore: adjusted path tooltip for repeating

Related to #796

* feat: repeated element validation

Related to #796

* wip: custom editor list renderer (missing icon)

Related to #796

* wip: collapse / expand dynamic lists

Related to #796

* feat: implement group/list alignment

Related to #796

* chore: added empty states to group and list

Related to #796

* feat: brought in new icons and matched visuals

Related to #796

* fix: form root outline never shows

* fix: editor no longer refreshes on element hover

Closes #807
Related to #796

* feat: add/remove items in dynamic lists

Related to #796

* feat: local expression contexts

Related to #796

* feat: add/remove styles

Related to #796

* feat: automatically scroll when adding item

Related to #796

* feat: refine add/remove UX

Related to #796

* chore: use karma-spec reporter for tests

* chore: refactored viewer and editor tests

Related to #808

* chore: large rebase test cleanups

Related to #808

* fix(submit): only modify error object when needed

Related to #808

* fix: final repeated subforms test adjustments

Related to #808

* chore: fixed minor tabs and naming issues

Related to #808

* feat: added carbon styles for dynamic list

Related to #907

* fix: improved dynamic list button outlines

Related to #808

* chore: rebase changes on `develop`

* chore: reduced nesting and improved docs in Form.js core functions

Related to #808

* chore: removed pointless useMemo

Related to #808

* chore: cleanup remnants of ai generation

Related to #808

* chore: switch from hardcoded to computed componentCount test
Related to #808

* fix(performance): centralize variable filtering

Related to https://github.com/camunda/tasklist/issues/3758

* chore: optimize svgs

Related to #808

* feat: implemented proper path management for repeatable fields

Related to #808

* fix: always expand when uncollapsible

Related to #808

* fix: proper key change handling and repeatable filtering

Related to #808

* feat: implement proper variable hiding within repeated field

Related to #808

* chore: adjust table/iframe tests following rebase

Related to #808

* fix: adjust numerical configs of dynamic list to prevent crashes

Related to #808

* fix: properly split path replacements in pathEntry

Related to #808

* fix: ensure dynamic list buttons don't submit

Related to #808

* feat: Carbonise dynamic list

* fix: show proper default uncollapsed items in propspan

Related to #808

* feat(schema): added dynamic list component

Related to #808

* chore(schema): adjust test indexes

Related to #808

---------

Co-authored-by: Niklas Kiefer <[email protected]>
Co-authored-by: Vinicius Goulart <[email protected]>

* chore: update actions/setup-node digest to 7247617

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Niklas Kiefer <[email protected]>
Co-authored-by: Vinícius Goulart <[email protected]>
Co-authored-by: bpmn-io-bot <[email protected]>
Co-authored-by: Valentin Serra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants