Cleanup datatable by TheGostKasper · Pull Request #4091 · weaveworks/weave-gitops

TheGostKasper · 2023-10-18T14:47:43Z

What changed?

To summarize, DataTable should not have any state itself, but instead force the parent to maintain the state. so that i choose to not change DataTable itself as it's used allover the system so TableVIew is the clone DataTable which display [header/body/emptyRow] ...etc

jpellizzari

Loving the way this is broken up.

Does it make sense to write some basic tests for this? Mainly just around the batch select logic, since that is the only significant complexity.

jpellizzari · 2023-10-23T19:35:26Z

This is called modal.ts but seems to have a lot of different types. Is that file name intentional?

jpellizzari · 2023-10-23T19:39:07Z

+  return (
+    <TableContainer id={id}>
+      <Table aria-label="simple table">
+        <TableHeader


Really nice work on making this nice an readable 👍

jpellizzari · 2023-10-23T19:40:33Z

+  searchedNamespaces: SearchedNamespaces;
+}
+
+const SearchedNamespacesModal = ({ searchedNamespaces }: Props) => {


Not understanding where this is used. Where in the UI does this modal appear?

Used in AutomationsTable and SourcesTable

TheGostKasper · 2023-10-24T12:50:53Z

Tests are covered in both DataTable and DataTableFilters 👌

jpellizzari

Really nice work 👍

opudrovs · 2023-10-30T14:02:58Z

+  };
+  return (
+    <TableContainer id={id}>
+      <Table aria-label="simple table">


❔ Is this aria-label informative to the user? It's a "simple table" for developers (comparing to the old all in one table), but for users it's just a table. And do we need an aria-label here at all?

From MDN:

Note: While aria-label is allowed on any element that can have an accessible name, in practice, aria-label is only supported on interactive elements, widgets, landmarks, images and iframes.

Is the whole table considered an interactive element?

Maybe we could add some roles instead of or in addition to aria-label?
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/table_role

I'll remove the aria-label

opudrovs · 2023-10-30T14:07:24Z

+        return (
+          <TableRow key={r.uid || i}>
+            {hasCheckboxes && (
+              <TableCell style={{ padding: "0px" }}>


❔ Is "px" redundant here? We usually just use padding: 0, AFAIR.

opudrovs · 2023-10-30T14:12:56Z

+          className={sortedField.reverseSort ? "upward" : "downward"}
+        />
+      ) : (
+        <div style={{ width: "16px" }} />


❌ Can we use the standard base standard spacing constant from the theme here instead of the hardcoded "16px" value? We are already using size="base" for the icon.

I assume hardcoded "16px" is fine here as it doesn't worth the change of creating a styled div with base or to import the them to the file to use only the value

opudrovs · 2023-10-30T14:22:50Z

+
+              return (
+                <TableCell
+                  style={Object.keys(style).length > 0 ? style : undefined}


✨ Neat check!

opudrovs · 2023-10-30T14:32:40Z

+  onBatchCheck,
+}: TableViewProps) => {
+  const onCheckChange = (checked: boolean, id: string) => {
+    const chk = [...checkedFields];


💡 I think we could use a more informative variable name here (preferably in plural) — to reflect that this variable holds an array of strings.

i'll rename it to selectedFields 🆗

opudrovs

Tested it locally, works fine for me. Great job! 🎉

* cleanup datatable * sort filtered items by `sortValue || value`

* fix duplicate icons * policy icon size * fix audit icon size * update icon * refactor: refactor away from deprecated wait.Poll calls With the update, some other calls had to be updated: - `NewDiscoveryRESTMapper` expects an extra `HTTPClient` argument - `client` does not have `NewDelegatingClient` anymore, instead we can create the same resource with `client.New(...)` Resolves #3812 References: - #3812 - kubernetes-sigs/controller-runtime#2150 - https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 Signed-off-by: Balazs Nadasdi <balazs@weave.works> * try to update github.com/fluxcd/pkg/runtime to the earliest version with updated controller-runtime Signed-off-by: Balazs Nadasdi <balazs@weave.works> * Cleanup datatable (#4091) * cleanup datatable * sort filtered items by `sortValue || value` * Update pkg/run/session/connect/connect.go Co-authored-by: Yiannis Triantafyllopoulos <8741709+yiannistri@users.noreply.github.com> * Refactoring Status column (#4098) * fix: Remove GitOps Run CLI commands * Replace the Sync/Suspend/Resume controls, used in the `SyncActions` and `CheckboxActions` components, with the new Sync/Suspend/Resume controls (the `SyncControl` component) (#4080) * Create the new `SyncControls` component for Sync/Suspend/Resume controls. * Move all components, related to syncing and suspending objects (existing `SyncActions` and `CheckboxActions` and new `SyncControls` and `ResumeIcon`), to the `Sync` folder. * Update the related UI snapshot. * Add `SyncControls` to exports. * Move custom actions to the start (left) of `SyncControls` buttons. * Re-arrange icons in `IconType` alphabetically. * add new svg icon as CLusterDiscovery icon * fix typo * update import order * build(deps): Bump google.golang.org/grpc from 1.51.0 to 1.56.3 Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.51.0 to 1.56.3. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.51.0...v1.56.3) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * build(deps): Bump github.com/weaveworks/tf-controller/api version * build(deps): Bump postcss from 8.4.21 to 8.4.31 in /website Bumps [postcss](https://github.com/postcss/postcss) from 8.4.21 to 8.4.31. - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.4.21...8.4.31) --- updated-dependencies: - dependency-name: postcss dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> * chore: Remove GitOps Run components * build(deps): Remove gitops bucket server from build * chore: Remove unused code previously used for GitOps Run * build(deps): Bump @babel/traverse from 7.20.13 to 7.23.2 in /website Bumps [@babel/traverse](https://github.com/babel/babel/tree/HEAD/packages/babel-traverse) from 7.20.13 to 7.23.2. - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.23.2/packages/babel-traverse) --- updated-dependencies: - dependency-name: "@babel/traverse" dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> * fix duplicate icons * remove extra space --------- Signed-off-by: Balazs Nadasdi <balazs@weave.works> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Joshua Israel <joshua.israel@weave.works> Co-authored-by: Balazs Nadasdi <balazs@weave.works> Co-authored-by: a.shabaan <ahmed.shabaan@weave.works> Co-authored-by: Yiannis Triantafyllopoulos <8741709+yiannistri@users.noreply.github.com> Co-authored-by: Luiz Filho <luizbafilho@gmail.com> Co-authored-by: yiannis <yiannis@weave.works> Co-authored-by: opudrovs <opudrovs@gmail.com> Co-authored-by: ahussein3 <ahmed.magdy@weave.works> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

cleanup datatable

47b1a06

TheGostKasper marked this pull request as draft October 18, 2023 14:47

sort filtered items by sortValue || value

4e25129

TheGostKasper mentioned this pull request Oct 18, 2023

Refactor DataTable weaveworks/weave-gitops-enterprise#3427

Closed

TheGostKasper added the area/ui Issues that require front-end work label Oct 22, 2023

run prettier

d256dba

TheGostKasper requested review from AlinaGoaga, joshri and opudrovs October 22, 2023 10:53

Merge branch 'main' into clean-datatable

0a9dad7

TheGostKasper marked this pull request as ready for review October 22, 2023 10:54

TheGostKasper added 2 commits October 22, 2023 15:50

fix unit tests

9d2bd89

ui-prettify-format

5b9f7d7

TheGostKasper changed the title ~~WIP - Cleanup datatable~~ Cleanup datatable Oct 23, 2023

TheGostKasper requested a review from jpellizzari October 23, 2023 13:35

jpellizzari reviewed Oct 23, 2023

View reviewed changes

update modal name to types

dbe3a4d

make prettier

f7dd414

jpellizzari approved these changes Oct 24, 2023

View reviewed changes

TheGostKasper added 2 commits October 26, 2023 15:08

Merge branch 'main' into clean-datatable

b6183ff

Merge branch 'main' into clean-datatable

4112a39

opudrovs reviewed Oct 30, 2023

View reviewed changes

opudrovs approved these changes Oct 30, 2023

View reviewed changes

apply comments

02ba299

snap

9b94e3f

TheGostKasper merged commit 15909de into main Oct 31, 2023

TheGostKasper deleted the clean-datatable branch October 31, 2023 12:02

AsmaaNabilBakr pushed a commit that referenced this pull request Nov 6, 2023

Cleanup datatable (#4091)

182e873

* cleanup datatable * sort filtered items by `sortValue || value`

weave-gitops-bot mentioned this pull request Nov 9, 2023

Updates for 0.36.0 #4127

Merged

weave-gitops-bot mentioned this pull request Jan 2, 2025

WIP - Do not merge - Updates for 0.39.0-rc.1 #4370

Closed

Conversation

TheGostKasper commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpellizzari left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheGostKasper commented Oct 24, 2023

Uh oh!

jpellizzari left a comment

Choose a reason for hiding this comment

Uh oh!

opudrovs Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opudrovs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TheGostKasper commented Oct 18, 2023 •

edited

Loading

opudrovs Oct 30, 2023 •

edited

Loading