Skip to content

Conversation

Anemy
Copy link
Member

@Anemy Anemy commented Aug 2, 2022

COMPASS-5678 COMPASS-5679

This pull request enables the new toolbars in Compass. It removes the COMPASS_SHOW_NEW_TOOLBARS feature flag, and the previous toolbar code. The new toolbars are built using LeafyGreen UI components.

Now the query history is opened as a popover managed by the query bar. This is a change in the query history's appRegistry role and how it's rendered. Previously it had a Collection.ScopedModal role. Now it has the role Query.QueryHistory and is rendered by the query bar, when showQueryHistoryButton is true (On cloud this is false). It's rendered using a Popover with a new hook in compass-components, useOnClickOutside.

This PR updates tests so they are now using these new toolbars.

Before:

before.mp4

After:

after.mp4

Before merging:

  • Sync w/ Claudia on the icon and button type (IconButton vs Button) for the export to language button. PR: feat(compass-query-bar): update export to language button and icon #3315
  • Sync w/ Claudia on the backgrounds and confirm changes are good (white toolbar background, scroll drop shadow).
  • Update the compass-crud toolbar buttons so the text doesn't wrap at low widths.
  • Notify cloud of the query bar changes that are now enabled and no longer feature flagged. There is a breaking change in the component api that will happen in the next packages release after this is merged. Previously we were accepting a layout prop. Now we accept a queryOptions prop. queryOptions are now an array of the inputs to render (excluding filter as that is always rendered). The previous prop layout had a different interface. @johnjackweir

<Icon glyph="Clock" />
<Icon glyph="CaretDown" />
</button>
<Popover
Copy link
Collaborator

Choose a reason for hiding this comment

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

This popover currently is not accessible. There is no exact aria example that would match this that I can link you here to, but basically what you have here is a composite element with an active descendant that is a popup that should manage its own focus, similar to an action menu or a toolbar (actually somewhere in between the two). At the very least it should assume focus on open and return it back to the trigger on close

Copy link
Member Author

Choose a reason for hiding this comment

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

Created a new component in compass-components called InteractivePopover. It abstracts the keyboard actions, rendering, and focus trap. If it looks good to you we'll also use this for rendering the saved aggregations list in COMPASS-5852
Let me know how that implementation looks, I'm adding tests now.
It uses react-focus-trap to take focus, and now we return focus to the trigger button when the popover is hidden.

Comment on lines 74 to 86
const onClickOutsideQueryHistory = useCallback(
(event) => {
// Ignore clicks on the query history button as it has its own handler.
if (
!queryHistoryButtonRef.current ||
queryHistoryButtonRef.current.contains(event.target as Node)
) {
return;
}
setShowQueryHistory(false);
},
[queryHistoryButtonRef, setShowQueryHistory]
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this apply to the whole group of components including button and popover? The way you are doing special case handling for the button here kinda negates the point of having a special hook for this use case

Copy link
Member Author

@Anemy Anemy Aug 2, 2022

Choose a reason for hiding this comment

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

Removed this exported hook in the new InteractivePopover component: #3312 (comment)
Has this comment gone away?

@mcasimir
Copy link
Collaborator

mcasimir commented Aug 2, 2022

(likely will land in a separate pr linked here).

Would it be possible to rather finish this work to the point that there is no further follow-up before we enable the FF?

@Anemy
Copy link
Member Author

Anemy commented Aug 2, 2022

@mcasimir Once this pr merges, the feature flag is gone. So we merge this when the new toolbars are ready to go.

@mcasimir
Copy link
Collaborator

mcasimir commented Aug 3, 2022

@mcasimir Once this pr merges, the feature flag is gone. So we merge this when the new toolbars are ready to go.

In general, I would prefer if the first PR that we merge with the new QB is with the FF enabled and not with the old code removed, but I also don't think we are ready to enable it just yet.

I've scheduled a sync with Claudia later today to talk about this. I'd rather keep the FF around and keep merging small PRs with improvements and fixes than keep the entire work here as an "all or nothing". I don't feel we can already make a commitment to finalize the work with this PR and release it right away.

Changing the QB is a big thing for both Compass and DE and I'd be hesitant to enable this FF right away. We may need to validate some aspects of this from a design perspective, and we probably want to give ourselves some time to manually test the interaction.

Having the new QB on main enabled while we still don't know if we should do more work on it may block releases .. and we have at least one planned for M1.

@Anemy
Copy link
Member Author

Anemy commented Aug 3, 2022

Synced w/ Claudia and Maurizio. Going to separate this into separate prs, we're going to hold the feature flag until we've done a few more changes. We'll also separate the removing old code and the removing feature flag, at least for a little to help ensure we have the old query bar ready if we need it.

@Anemy Anemy closed this Aug 3, 2022
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