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-workspace): breadcrumbs COMPASS-7593 #5501

Merged
merged 21 commits into from
Mar 2, 2024
Merged

Conversation

mabaasit
Copy link
Contributor

@mabaasit mabaasit commented Feb 26, 2024

This PR adds breadcrumbs to Compass workspace. It also remove collection information from the header and its shown now in the corresponding tab names (see video).

Workspace Breadcrumbs
Screen.Recording.2024-02-26.at.23.21.17.mov
Collection Meta
Screen.Recording.2024-02-26.at.23.22.38.mov

Description

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)

@mabaasit mabaasit marked this pull request as ready for review February 26, 2024 22:29
return (
<div
title={`${database}.${collection}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: you already have a "composed" namespace here

Suggested change
title={`${database}.${collection}`}
title={namespace}

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'd be remove this completely as it adds title to the whole breadcrumb and the badges within this div.

export const Breadcrumbs = ({ items }: { items: Array<BreadcrumbItem> }) => {
const darkMode = useDarkMode();
return (
<div className={breadcrumbStyles} data-testid="breadcrumbs">
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to have some sort of overflow control here:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 7f402b9

<>
<Link
key={item.name}
as="a"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we're not handling this as a link and there's no url, this should be as button, right?

Suggested change
as="a"
as="button"

const breadcrumbItems = useMemo(() => {
return [
{
// TODO (COMPASS-7684): add connection name
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 if it's worth discussing with design, but while we wait for the multiconnection work to handle this, maybe we can hide it for now?

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'll check it with the design and update this (and the todo ticket) accordingly.

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 removed it from this PR in this b95a406. I added this for reference in the ticket. So now we are only showing breadcrumb on the collection-header, without Cluster

packages/databases-collections-list/src/items-grid.tsx Outdated Show resolved Hide resolved
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.

Looks good, awesome to see all the extra space in the header! Left two small comments, but nothing blocking

name: toNS(namespace).collection,
onClick: () => openCollectionWorkspace(namespace),
},
// When editing a view, show the view namespace last
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 if it's something that needs addressing, but just want to call out that this logic doesn't match the logic in the workspace tab breadcrumb. Do we need to update it too?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a really good point. I'll check with design and if needed create a follow up.

collectionMetadata.isTimeSeries ||
collectionMetadata.sourceName ||
editViewName;
if (!hideStats && tab.name === 'Documents') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super nit, if you first check for hideStats, you don't need to do it here and in the check below, seems to be that it reads a bit easier that way:

if (alwaysHideStats) {
  return tab.name;
}
if (tab.name === 'Documents') {
  ...

@mabaasit mabaasit merged commit 37d0c01 into main Mar 2, 2024
13 of 15 checks passed
@mabaasit mabaasit deleted the breadcrumbs branch March 2, 2024 06:04
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.

3 participants