Skip to content

feat(sidebar): show header for Data Explorer COMPASS-8904 #6711

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 12, 2025
Merged

feat(sidebar): show header for Data Explorer COMPASS-8904 #6711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f09aed
(feat)sidebar: show header for Data Explorer COMPASS-8904
nvs119 Feb 11, 2025
a44c559
add preview badge
nvs119 Feb 11, 2025
952f380
styling
nvs119 Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions packages/compass-sidebar/src/components/multiple-connections/header/sidebar-header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,28 @@ import {
css,
type ItemAction,
ItemActionControls,
Badge,
BadgeVariant,
} from '@mongodb-js/compass-components';

const sidebarHeaderStyles = css({
paddingLeft: spacing[400],
paddingRight: spacing[400],
display: 'flex',
justifyContent: 'space-between',
alignItems: 'center',
Copy link
Contributor Author

@nvs119 nvs119 Feb 11, 2025

Choose a reason for hiding this comment

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

I kept it with justifyContent: 'space-between' because I want to keep the settings icon all the way to the right when it exists, so there's a slight difference between the design in the ticket (badge is closer to the Subtitle) vs how it'll actually be rendered (all the way to the right of the header). Is that okay?

Copy link
Collaborator

@gribnoysup gribnoysup Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You can just move the badge inside the subtitle and add some styling to position it to make it aligned with the designs, something like:

const badgeStyles = css({
  vertical-align: super;
  margin-left: spacing[100];
});

// ...

<Subtitle className={sidebarHeaderTextStyles}>
  {isCompassWeb ? 'Data Explorer' : 'Compass'}
  {isCompassWeb && (
    <Badge
      variant={BadgeVariant.Blue}
      className={badgeStyles}
      data-testid="sidebar-header-badge"
    >
      Preview
    </Badge>
  )}
</Subtitle>

Copy link
Contributor Author

@nvs119 nvs119 Feb 12, 2025

Choose a reason for hiding this comment

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

thank you! Changed now

});

const sidebarHeaderTextStyles = css({
lineHeight: '32px',
fontWeight: 600,
});

const badgeStyles = css({
verticalAlign: 'middle',
marginLeft: spacing[100],
});

type Action = 'open-compass-settings';

const actions: ItemAction<Action>[] = [
Expand All @@ -31,18 +39,33 @@ const actions: ItemAction<Action>[] = [

export function SidebarHeader({
onAction,
isCompassWeb,
}: {
onAction(actionName: Action): void;
isCompassWeb?: boolean;
Copy link
Collaborator

@gribnoysup gribnoysup Feb 12, 2025

Choose a reason for hiding this comment

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

I'm a bit hesitant about adding a property like that to the component in the same spirit of not making it too easy to differenciate by environment like that as this goes against the architectural goals of the project. I would sugggest to either change the prop to be more aligned with the actual behavior that we want to allow to configure (like making a sidebarTitle prop with a default value that would allow compass-web to override it with its own component + some prop to hide settings button explicitly) or to use AtlasClusterConnectionsOnly context that Dan added recently to drive this behavior because the use-cases for it and this change that we want to do are exactly the same.

This is sort of splitting hair, so you tell me if changing this makes sense to you

Copy link
Contributor Author

@nvs119 nvs119 Feb 12, 2025

Choose a reason for hiding this comment

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

discussed offline, leaving as is for now

}): React.ReactElement {
return (
<div className={sidebarHeaderStyles} data-testid="sidebar-header">
<Subtitle className={sidebarHeaderTextStyles}>Compass</Subtitle>
<ItemActionControls<Action>
onAction={onAction}
iconSize="small"
actions={actions}
data-testid="connections-sidebar-title-actions"
></ItemActionControls>
<Subtitle className={sidebarHeaderTextStyles}>
{isCompassWeb ? 'Data Explorer' : 'Compass'}
{isCompassWeb && (
<Badge
variant={BadgeVariant.Blue}
className={badgeStyles}
data-testid="sidebar-header-badge"
>
Preview
</Badge>
)}
</Subtitle>
{!isCompassWeb && (
<ItemActionControls<Action>
onAction={onAction}
iconSize="small"
actions={actions}
data-testid="connections-sidebar-title-actions"
></ItemActionControls>
)}
</div>
);
}
19 changes: 10 additions & 9 deletions packages/compass-sidebar/src/components/multiple-connections/sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ type MultipleConnectionSidebarProps = {
activeWorkspace: WorkspaceTab | null;
onConnectionCsfleModeChanged(connectionId: string, isEnabled: boolean): void;
onSidebarAction(action: string, ...rest: any[]): void;
showSidebarHeader?: boolean;
onOpenConnectViaModal?: (
atlasMetadata: ConnectionInfo['atlasMetadata']
) => void;
isCompassWeb?: boolean;
};

const sidebarStyles = css({
Expand Down Expand Up @@ -92,8 +92,8 @@ export function MultipleConnectionSidebar({
activeWorkspace,
onSidebarAction,
onConnectionCsfleModeChanged,
showSidebarHeader = true,
onOpenConnectViaModal,
isCompassWeb,
}: MultipleConnectionSidebarProps) {
const [csfleModalConnectionId, setCsfleModalConnectionId] = useState<
string | undefined
Expand Down Expand Up @@ -167,13 +167,14 @@ export function MultipleConnectionSidebar({
return (
<ResizableSidebar data-testid="navigation-sidebar" useNewTheme={true}>
<aside className={sidebarStyles}>
{showSidebarHeader && (
<>
<SidebarHeader onAction={onSidebarAction} />
<Navigation currentLocation={activeWorkspace?.type ?? null} />
<HorizontalRule />
</>
)}
<>
<SidebarHeader
onAction={onSidebarAction}
isCompassWeb={isCompassWeb}
/>
<Navigation currentLocation={activeWorkspace?.type ?? null} />
<HorizontalRule />
</>
<ConnectionsNavigation
connectionsWithStatus={connectionsWithStatus}
activeWorkspace={activeWorkspace}
Expand Down
6 changes: 3 additions & 3 deletions packages/compass-sidebar/src/plugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@ const errorBoundaryStyles = css({
});

export interface SidebarPluginProps {
showSidebarHeader?: boolean;
onOpenConnectViaModal?: (
atlasMetadata: ConnectionInfo['atlasMetadata']
) => void;
isCompassWeb?: boolean;
}

const SidebarPlugin: React.FunctionComponent<SidebarPluginProps> = ({
showSidebarHeader,
onOpenConnectViaModal,
isCompassWeb,
}) => {
const activeWorkspace = useActiveWorkspace();
const { log, mongoLogId } = useLogger('COMPASS-SIDEBAR-UI');
Expand All @@ -41,9 +41,9 @@ const SidebarPlugin: React.FunctionComponent<SidebarPluginProps> = ({
}}
>
<MultipleConnectionSidebar
showSidebarHeader={showSidebarHeader}
activeWorkspace={activeWorkspace}
onOpenConnectViaModal={onOpenConnectViaModal}
isCompassWeb={isCompassWeb}
/>
</ErrorBoundary>
);
Expand Down
2 changes: 1 addition & 1 deletion packages/compass-web/src/entrypoint.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,8 @@ function CompassWorkspace({
renderSidebar={() => {
return (
<CompassSidebarPlugin
showSidebarHeader={false}
onOpenConnectViaModal={onOpenConnectViaModal}
isCompassWeb={true}
Copy link
Contributor Author

@nvs119 nvs119 Feb 11, 2025

Choose a reason for hiding this comment

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

can't figure out why this is giving me type error:
Type '{ onOpenConnectViaModal: ((atlasMetadata: AtlasClusterMetadata | undefined) => void) | undefined; isCompassWeb: boolean; }' is not assignable to type 'IntrinsicAttributes & SidebarPluginProps & { children?: ReactNode; }'. Property 'isCompassWeb' does not exist on type 'IntrinsicAttributes & SidebarPluginProps & { children?: ReactNode; }'.ts(2322)

Copy link
Contributor Author

@nvs119 nvs119 Feb 11, 2025

Choose a reason for hiding this comment

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

just kidding! running npm run bootstrap fixed this - was there a less heavyweight command i could run to reload types?

Copy link
Collaborator

@gribnoysup gribnoysup Feb 12, 2025

Choose a reason for hiding this comment

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

Yeah, sorry, we didn't touch on that, for context: types are pulled from compiled files, so if you change one package, you need to rebuild it so that you get correct highlight in the other. You can manually rebuild just one package by running something like npm run bootstrap -w packages/<package dir>, this will avoid rebuilding everything like bootstrap does

></CompassSidebarPlugin>
);
}}
Expand Down
Loading