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(compass-global-writes): zone table - add search and collapse/expand COMPASS-8336 #6356

Merged
merged 13 commits into from
Oct 23, 2024

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Oct 11, 2024

Description

LG Table is based on https://tanstack.com/table/latest. Here we're taking use of it to add search, filter and nested rows.

Expanding logic is customized, so that the parents auto-expand to display matching children. I'm not sure why this is not the default.

Side changes:

  • removed max width on some elements, so that the layout is consistent with the other tabs
  • added global-writes to the compass-web uri builder

The filtering tests do not work for electron. As Global writes are for now only planned for web, I am skipping them for now and created https://jira.mongodb.org/browse/COMPASS-8368 to tackle this later.

Screenshot 2024-10-14 at 15 49 17

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@paula-stacho paula-stacho changed the title Compass 8336 COMPASS-8836 Oct 11, 2024
@paula-stacho paula-stacho changed the title COMPASS-8836 COMPASS-8336 Oct 14, 2024
@paula-stacho paula-stacho changed the title COMPASS-8336 feat(compass-global-writes): zone table - add search and collapse/expand COMPASS-8336 Oct 14, 2024
@github-actions github-actions bot added the feat label Oct 14, 2024
@paula-stacho paula-stacho added the no release notes Fix or feature not for release notes label Oct 14, 2024
@paula-stacho paula-stacho marked this pull request as ready for review October 14, 2024 13:55
@johnjackweir johnjackweir requested review from johnjackweir and removed request for johnjackweir October 15, 2024 19:23
@@ -62,7 +62,8 @@
"react": "^17.0.2",
"react-redux": "^8.1.3",
"redux": "^4.2.1",
"redux-thunk": "^2.4.2"
"redux-thunk": "^2.4.2",
"@tanstack/table-core": "^8.14.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that we're using it only in global-writes, but just from the perspective of being able to easier maintain matching versions of UI dependencies, do you mind moving this to compass-components and re-exporting from there? Otherwise it will be hard to track where to make version updates next time we update leafygreen table along other leafygreen deps

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be ideal, yeah, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved the imports! this time I looked into LG and found out they actually do re-export tanstack table, I just didn't see it because some parts like the row type are overridden. now that I only need the type I thought it could even be just dev dep, but that didn't pass depcheck for some reason 🤔

};

const hasFilteredChildren = (
row: LgTableRowType<LGTableDataType<ShardZoneRow>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is my understanding here correct that the LG table component is based on top of the @tanstack/table-core package and we're only using the latter because LG doesn't expose a type like it? Do you think it's clear enough what the different between e.g. LgTableRowType and LeafygreenTableRow is?

If that's the case, I'd assume that LG either doesn't re-export relevant types by accident (i.e. we could ask them for that in a ticket and leave a TODO comment) or intentionally (in which case we shouldn't be using them and considering them an implementation detail, right?).

We could get away with a more descriptive type here as an alternative, e.g.

Suggested change
row: LgTableRowType<LGTableDataType<ShardZoneRow>>
row: { subRows: { columnFilters: Record<string, unknown> }[] }

Copy link
Contributor Author

@paula-stacho paula-stacho Oct 23, 2024

Choose a reason for hiding this comment

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

So I was digging into this and they do reexport the tanstack table, the trouble is some of the exports (like the Row) get overridden by other exports (LG table defines it's own Row). The LG team said they are doing some refactor on the table right now, so they'll look into this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright – then I'd add a TODO comment with that reference, or go with something like the suggestion above here (I think that's perfectly fine typing for this situation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added small ticket for us, linked to their refactor ticket: https://jira.mongodb.org/browse/COMPASS-8437 (waiting for confirmation from Shaneeza that I got the correct ticket)

Comment on lines +68 to +69
if (typeOneIsoCode === isoCode) {
Object.assign(groups[typeOneIsoCode], parseRow(next));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I understand which case this is handling: wouldn't this already be covered by the initial groups[typeOneIsoCode] assignment above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if we can count on the order they come in. If the first zone that comes in is not the top level (typeOneIsoCode !== isoCode), then the assignment above is a placeholder and will be replaced by the top level here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, this detail escaped me when reading through the logic, I see it now, didn't really connect that the parseRow we're doing there might not be the actual "type one" code, makes sense! Do you think the parseRow above is needed then? Are we adding it only to make typescript happy?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Almost want to suggest to explicitly assign something like { locationName: typeOneIsoCode, zone: typeOneIsoCode } there just so it's clear that this is sort of a placeholder for the top level zone before we can assign something meaningful there, but I'll leave to you to decide, it's a small thing

@paula-stacho paula-stacho merged commit 2ec13eb into main Oct 23, 2024
28 checks passed
@paula-stacho paula-stacho deleted the COMPASS-8336 branch October 23, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat no release notes Fix or feature not for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants