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

fix: integrate connections into the sidebar navigation tree COMPASS-7773 #5615

Merged
merged 52 commits into from
Apr 10, 2024

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Mar 22, 2024

TODO:

  • Server icon
  • Fix active workspace highlighting
  • Click on a connection
  • New tests for the navigation tree
  • Check figma for actual leftPadding values for the tree items

Description

=== READ FIRST ===

The focus of this piece is purely on the presentation.

That means

  • the changes in compass-sidebar are mainly preparation. what really matters at this point is that we're not breaking the legacy sidebar
  • we're reusing the same list of databases for each connection
  • activation of the sidebar items based on activeNamespace is not sufficient because it doesn't include connection, we'll need something like activeConnection alongside it

The actual state management will be implemented in https://jira.mongodb.org/browse/COMPASS-7775

Changes:

  • compass-databases-navigation changes to compass-connections-navigation
  • the legacy usage is preserved, with databasesLegacy and expandedLegacy parameters. This is also covered by the original set of tests, now legacy-connections-navigation-tree.spec.tsx
  • the new top level is connections
  • clicking on a connection item opens the databases tab (openDatabasesWorkspace). focusing on the tab later activates the connection item
  • connections-navigation-tree.tsx is an updated copy of databases-navigation-tree.tsx. I can only recommend viewing the diff between the two files, unfortunately github doesn't do this (same for the .spec file)
  • similar case is connection-item.tsx, which is based on database-item.tsx

Missing: In the new designs, the icons are lighter and the highlighting has a different color. I ran out of time (and the PR is already very big), so looks like this will be a follow up.

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 7773 fix: integrate connections into the sidebar navigation tree COMPASS-7773 Mar 22, 2024
@github-actions github-actions bot added the fix label Mar 22, 2024
@paula-stacho paula-stacho added the feature flagged PRs labeled with this label will not be included in the release notes of the next release label Mar 22, 2024
Base automatically changed from COMPASS-7654 to main March 25, 2024 15:50
@paula-stacho
Copy link
Contributor Author

Note: I made the namespace parameter for databases workspace required, forcing a bunch of namespace: '' placeholders. I have since started to doubt this decision, especially knowing that DE will be working with single connections for the time being. I'll probably change it to optional.

@paula-stacho paula-stacho marked this pull request as ready for review March 26, 2024 16:47
Comment on lines +3 to +4
// TODO: Currently we show placeholder for every collection/database item in the list, but
// do we want to / need to?
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a deliberate design decision to prevent collection list and the content below it from "jumping" when collections finished loading. Why we'd want to remove that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, I believe the current behaviour is the correct one. I'll revert this change so we keep the placeholder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, what did we want to revert here? or is it reverted already? @gribnoysup @kmruiz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: this file is moved from compass-databases-navigation

Copy link
Contributor

Choose a reason for hiding this comment

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

I think nothing needs to be reverted, looking at what the code does, it still keeps the placeholders as expected.

Comment on lines 341 to 343
databasesLegacy?: Database[];
expanded?: Record<string, false | Record<string, boolean>>;
expandedLegacy?: Record<string, boolean>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like an unnecessary fork in logic here. We will need to support single connection in compass-web with the new sidebar: the logic is pretty straightforward, if there is only one connection and only one collection is allowed, this tree should render as before, can we just incorporate this logic into the component and not have a hard fork on this logic based on props? It's low effort and less cleaning-up afterwards when the multiple connections are enabled in Compass

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I'll do the change when I take this ticket.

Copy link
Contributor Author

@paula-stacho paula-stacho Apr 9, 2024

Choose a reason for hiding this comment

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

@gribnoysup not sure if I follow here.. is the idea just to keep the databases name and expanded type will depend on whether connections or databases are set? we can do that for sure, it just doesn't seem like a "fork in logic" for me, it's really just the props layer. that's why I'm not sure if I understand correctly.
when I started on this I wasn't aware of the compass-web single connection scenario, so I thought the cleanup would be easiest if we clearly mark the legacy bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also relevant note - the compass-web will likely keep using a separate sidebar because the new sidebar structure doesn't make sense for single connection. whether we call it legacy or single-connection (likely the latter, but I'm afraid to rename the sidebar folder at this moment, there are too many conflicts with the main branch already and at least one rebase last week apparently went wrong 🙈 )

Copy link
Contributor Author

@paula-stacho paula-stacho Apr 9, 2024

Choose a reason for hiding this comment

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

update: I've changed the props and also internally removed the "legacy" notation in favour of "single connection (SC)"

@@ -0,0 +1,32 @@
/* eslint-disable react/prop-types */
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen this disable directive in a couple of files here, it doesn't seem to trigger anything locally though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, might not be needed anymore. I'll try removing it and see if it triggers anything in the CI

@paula-stacho paula-stacho removed the wip label Apr 10, 2024
@paula-stacho paula-stacho merged commit e9e4942 into main Apr 10, 2024
12 of 15 checks passed
@paula-stacho paula-stacho deleted the COMPASS-7773 branch April 10, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature flagged PRs labeled with this label will not be included in the release notes of the next release fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants