feat(sidebar): show header for Data Explorer COMPASS-8904 #6711
Conversation
nvs119
changed the title
| <CompassSidebarPlugin | ||
| showSidebarHeader={false} | ||
| onOpenConnectViaModal={onOpenConnectViaModal} | ||
| isCompassWeb={true} |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
just kidding! running npm run bootstrap fixed this - was there a less heavyweight command i could run to reload types?
There was a problem hiding this comment.
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
| paddingRight: spacing[400], | ||
| display: 'flex', | ||
| justifyContent: 'space-between', | ||
| alignItems: 'center', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>There was a problem hiding this comment.
thank you! Changed now
| isCompassWeb, | ||
| }: { | ||
| onAction(actionName: Action): void; | ||
| isCompassWeb?: boolean; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
discussed offline, leaving as is for now
Description
Adding header back for Compass Web with a preview badge for private preview users
COMPASS-8904
Motivation and Context
Open Questions
Getting a type error on line 206 of compass-web/src/entrypoint.tsx, which I can't figure out why for the life of me. Help appreciated!!
Dependents
Types of changes