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

chore: factor out the inner workings of the indexes table so we can have separate regular and search ones COMPASS-7163 #4820

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Sep 7, 2023

So we'd have RegularIndexesTable and SearchIndexesTable and both reuse IndexesTable.

@lerouxb lerouxb marked this pull request as ready for review September 7, 2023 16:04
Comment on lines +18 to +22
rowStyles,
indexActionsCellStyles,
tableHeaderStyles,
cellStyles,
nestedRowCellStyles,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really a pattern we use often and leaks a bit more implementation detail than probably needed. Do you think it would be possible to hide this behind the interface of the table, so that it's still the one responsible for rendering rows and cells?

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 tried that at first but I found it awkward without the Row and Cell props exported by leafygreen. I suppose I can make them dumb and only accept a few props..

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 tried. Weird stuff happening in the Row component that I can't figure out so I reverted it again for now. I think we can try again in a follow-up and I'd like to move on for now and unblock people.

@lerouxb lerouxb force-pushed the refactor-indexes-table branch from dc1a0e9 to 94d540b Compare September 8, 2023 10:56
@lerouxb lerouxb force-pushed the refactor-indexes-table branch from 94d540b to 22227fe Compare September 8, 2023 11:05
Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

It's very weird how we are exporting parts of the table to make it reusable, please, let's open a ticket to sort this out. Approving because https://github.com/mongodb-js/compass/pull/4820/files#r1319760896 but this is really not the way to go about this

@lerouxb
Copy link
Contributor Author

lerouxb commented Sep 8, 2023

Filed a ticket: https://jira.mongodb.org/browse/COMPASS-7199

@lerouxb lerouxb merged commit 9c2af84 into main Sep 8, 2023
@lerouxb lerouxb deleted the refactor-indexes-table branch September 8, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants