-
Notifications
You must be signed in to change notification settings - Fork 246
feat(components, data-modeling): add portal version of the Drawer component; use new DrawerSection in data-modeling COMPASS-9610 #7138
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
Conversation
…ponent; use new DrawerSection in data-modeling
| // TODO: Leafygreen doesn't allow us to tie close event to a particular | ||
| // action. We can add this functionality ourselves, but I'm not sure that | ||
| // adding even more logic on top of the drawer is a good idea. Maybe we're | ||
| // okay with the drawer close button click just staying there until you | ||
| // explicitly click something else? | ||
| // onClose={onClose} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's kinda the only thing missing here, but as mentioned, I think it might be okay to just skip to avoid adding extra complexity to this system. Literally the next project for Data Modeling will make this redundant anyway because we will always have at least one section in the drawer (no selection becomes sort of "top level" selection) and so this button will behave as just "collapse" not "deselect" anyway.
I also can't see a use-case for providing this onClose interface in "AI Assistant" project
| toolbarData.length === 0 && emptyDrawerLayoutFixesStyles, | ||
| // classname is the only property leafygreen passes over to the drawer | ||
| // wrapper component that would allow us to target it | ||
| 'compass-drawer-anchor' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No data-testid? Should we ask for one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, nothing beyond className is getting passed, and not even that for children, so I have to weirdly target them through nth-child selectors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they want us to use the lgIds https://github.com/mongodb/leafygreen-ui/blob/main/packages/drawer/src/utils/getLgIds.ts
They said they're keeping the passing of data-testids only as a backwards compatibility thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These ids then end up on the actual drawer & toolbar elements and not the layout, which is too high
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify this particular case, this class name here is not used for tests, it's used for actual feature behavior, I need to target a specific element to be able to find the node in another component and a common pattern for that is to pass data-* attributes (not necessarily data-test-id, this is just a common convention for testing). They really shouldn't mess up the normal browser features like that 😢 I'll reach out to them
| }); | ||
| useLayoutEffect(() => { | ||
| const drawerEl = document.querySelector( | ||
| '.compass-drawer-anchor > div:nth-child(2)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be really nice if we could get some id or something to target this in a way that won't be so brittle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll be reaching out to lg team about this
paula-stacho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so cool! 😎
Only thing I'm not sure about is the order working in a z-index-y way. Might be it will not be an issue because we mostly won't care about the order, but it could become a potential pain point.
|
Yeah, that's a valid concern, maintaining the values across the codebase might become error prone if we have too much cases where the ordering is really important, hard to adjust for that while we only have one case, but let's keep this in mind! |
This patch adds a new component that wraps leafygreen Drawer to circumvent some issues with their component (I'll reach out to lg team about those) and, more importantly, add the behavior that we actually need in Compass and is not provided by this Drawer component:
In Compass drawer section might be rendered by any plugin at any point of their lifecycle, leafygreen component design as-is would force us to push all the state required to render drawers into a single place and this goes against the design principles of Compass. To address that we're introducing a portal Drawer, leaning into React guidelines about component composition over configuration, Reacts ability to tunnel React rendered components into arbitrary DOM nodes, declarative nature of React rendering.
The interface that we introduce here splits Drawer into three independent parts:
<DrawerContentProvider />top level component that just keeps the required UI state as high as possible in the React rendering tree, allowing us to render Drawer sections (probably less relevant) and access Drawer actions (way more relevant for us) outside of the leafygreenDrawerLayoutcomponent<DrawerAnchor />component that is reponsible for actually placing the leafygreen Drawer on the screen. Having this separate from any Drawer content means that we can easily move it around without the need for changes in any of the plugins that want to render Drawer sections<DrawerSection />component that gives plugins full control over rendering sections in the Drawer without the need to manually propagate any state outside of themselvesThe
DrawerContentProviderandDrawerAnchorcomponents are expected to be set up only once for the whole application. As this PR deals with all the initial setup, any other plugin that wants to render a arbitraryDrawersection now needs only to think about rendering theDrawerSectionbound to the local state (either local component state, or connected from the plugin store), while other parts of this Drawer system stay totally unaware of it.Instead of needing to provide all the section data for the Drawer in advance or manually juggle providing this state and opening Drawer at just the right time, we provide a
DrawerSectioncomponent that allows to declaratively define sections when needed. On every renderDrawerSectionwill sync it's props to the top levelDrawerContentProviderand will render its content into a special section anchor insideDrawerLayout(insideDrawerAnchor) viacreatePortalmethod.If needed, there is still programmatic control over the drawer that allows to open / close it via the
useDrawerActionshook.This is how it looks in the "Data Modeling" feature, notice how the code maps to behavior: declaratively rendered component drives the drawer opening with content, then when you switch to another tab, section unmounts, cleaning itself up from the drawer (right now this just removes the drawer completely).
I need to adjust unit tests a bit to account for rendering being async now, but otherwise this is working locally and passing checks. I'm opening this for review so that we can start looking at the interface design and discuss the rationale for this approach if needed.