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

Use new Fluent-based Resource Tree for all environments #1841

Merged
merged 19 commits into from
May 29, 2024

Conversation

analogrelay
Copy link
Contributor

@analogrelay analogrelay commented May 14, 2024

Preview this branch

This is part of our redesign work. At @languy 's suggestion, I decided it would be useful if we unified our Resource Tree components before redesigning the tree itself. That way we have a consistent view across all our environments.
 
When hosted in Fabric, we use a new "ResourceTree2" component that is based on Fluent UI 9. Currently, other environments (https://cosmos.azure.com, Portal, Emulator, etc.) all use the existing ResourceTree based on custom UI logic and Fluent UI 8.

This PR unifies all the environments to the newer ResourceTree2 component. I also removed the ResourceTokenTree and rolled its functionality into the tree-building logic that is used by ResourceTree2.

I then removed the old ResourceTree and renamed ResourceTree2 to just "ResourceTree". I wanted to do similar for the "TreeComponent" stuff it uses, but there are some lingering elements of it (see below) so for now I just renamed "TreeComponent" to "LegacyTreeComponent" and "TreeNode2Component" to "TreeNodeComponent" in preparation for removing the "legacy" tree components.

So, in summary:

Before After Notes
ResourceTree <deleted> "Old" custom resource tree
ResourceTokenTree <deleted> "Old" resource tree used when connecting with a Resource Token, merged with new ResourceTree
ResourceTree2 ResourceTree "New" Fluent UI 9 resource tree
ResourceTreeContainer ResourceTreeContainer Updated to only use the new "ResourceTree"
Any TreeN components LegacyTreeN Renamed "old" tree components to have Legacy prefix
Any TreeNode2N components TreeNodeN Renamed "v2" tree components to remove the "2"

NOTE: The renames are relatively isolated and can be undone or changed if folks have other preferences for how we do that!

I do have one open question: The ResourceTreeAdaptor component appears to be largely unused. It's only really used via the enableKoResourceTree feature flag, and appears to just be a left-over "backup" option to go back to the previous-generation knockout-based resource tree. I did also find reference to it in Notebooks-related components but I believe those are being deprecated/removed soon?

I've validated a number of scenarios here, comparing the new tree behavior with the existing tree:

  • Connecting with a Resource Token Connection String (I also updated the generateResourceToken.js helper as part of this, to make it a little easier to use)
  • NoSQL API in Hosted Explorer, Portal, Emulator
  • MongoVCore/Postgres in Portal (not super relevant, since there's no tree, but nothing is broken!)
  • Cassandra API in Hosted Explorer, Portal
  • Mongo API in Hosted Explorer, Portal
  • Gremlin API in Hosted Explorer, Portal
  • Tables API in Hosted Explorer, Portal

@analogrelay
Copy link
Contributor Author

analogrelay commented May 14, 2024

Mostly posting this now to get CI runs, plus we're avoiding merges at the moment anyway. But feel free to provide feedback and/or test it out!

Now ready for review!

@analogrelay analogrelay force-pushed the ashleyst/merge-resource-trees branch from 47ae7b1 to 074cbe4 Compare May 21, 2024 21:17
@analogrelay analogrelay marked this pull request as ready for review May 21, 2024 21:46
@analogrelay analogrelay requested a review from a team as a code owner May 21, 2024 21:46
languy
languy previously approved these changes May 22, 2024
test/utils/shared.ts Show resolved Hide resolved
jawelton74
jawelton74 previously approved these changes May 22, 2024
@jawelton74
Copy link
Contributor

The difference in font size between resource tree and item list is a little jarring - is that intentional ?

image

@jawelton74 jawelton74 closed this May 22, 2024
@sevoku
Copy link
Contributor

sevoku commented May 22, 2024

The difference in font size between resource tree and item list is a little jarring - is that intentional ?

It looks a little bit bigger indeed, we should make sure the font sizes fit together with the new fluent documents list: #1770, since we want to get both in right after Build?

@jawelton74 jawelton74 reopened this May 22, 2024
@analogrelay
Copy link
Contributor Author

The difference in font size between resource tree and item list is a little jarring

@jawelton74 @sevoku It's not intentional, I can patch it up. It's worth noting there is a full redesign of the resource tree on the queue as well (which should be consistent with the document list view), but a simple font-size fix-up is definitely worth doing before this merges. Once both new views have landed, the redesign work should help get things more consistent (using shared stylings, etc.), but it's absolutely possible to make some quick patches now before this lands to make things less jarring.

@analogrelay
Copy link
Contributor Author

Looking at the new Fluent Resource Tree, I think it actually matches the font sizes being used here (which makes some sense, since they are both mostly using Fluent's default styling. I'm going to merge the two branches locally and do some testing (both in Hosted/Portal and Fabric) to confirm that, but I think that with both of these changes landed, the font sizes will line up. Screenshots to come shortly!

@analogrelay
Copy link
Contributor Author

Ok, so there is still a discrepancy. If we just merge #1770 and this PR (unmodified), there's a fairly noticable size difference between Tree items and Table items:

image

So, I'm sizing the Tree down to the small size (it's currently at the default medium size). Which should align things better (the Table component in #1770 is set to extra-small, which has the same font size as the Tree component's small):

image

This does make the tree a little smaller than it currently is on Fabric, which I could undo with a few simple ifs if needed. Currently, this is how it looks on Fabric:

image

And with this change and no ifs it would look like this:

image

I'm inclined to leave it that way, since we're doing more redesign passes on this soon.

So, to summarize:

  • I'm about to make a change to reduce the Tree font size to better align with both the existing Items list and the new one
  • This also reduces the Tree font size in Fabric (but makes it consistent with Hosted/Portal)
  • We'll be making another pass soon to do a full redesign of the tree, and can continue refining things in that pass.

@analogrelay analogrelay dismissed stale reviews from jawelton74 and languy via c3ce884 May 23, 2024 18:48
jawelton74
jawelton74 previously approved these changes May 23, 2024
languy
languy previously approved these changes May 24, 2024
@languy
Copy link
Member

languy commented May 24, 2024

I'm inclined to leave it that way, since we're doing more redesign passes on this soon.

I vote to leave it that way as well. We keep the differences between Azure and Fabric to a minimum (ie the colors) to make it clear it's the same Data Explorer and don't confuse the customer. And it's easier to maintain.

@analogrelay analogrelay dismissed stale reviews from languy and jawelton74 via 1932056 May 28, 2024 17:22
@analogrelay analogrelay force-pushed the ashleyst/merge-resource-trees branch from c3ce884 to 1932056 Compare May 28, 2024 17:22
@analogrelay
Copy link
Contributor Author

Update! Rebased on master and verified things still work as expected. There is one additional functional change: In reviewing the docs for the Fluent UI Tree component, there is an important accessibility warning:

⚠️ Although actions are easy to navigate, they're not an expected pattern according to WAI-ARIA.providing a context menu with the same functionalities as the actions is recommended to ensure your tree item is accessible.

https://react.fluentui.dev/?path=/docs/components-tree--default#actions

So, in order to do that, I've added code to duplicate the actions menu into the context menu for each tree node item. This means you can right-click on a tree node to get the same menu as you would when clicking the "..." button.

However, in order to make this work, I had to remove a global binding to contextmenu on document that suppresses the browsers default context menu on the Data Explorer (see below for why, it's a React quirk). I think this is also a good change. I don't think it adds user value here to suppress the default context menu. I don't have context on why we suppressed the context menu though. I looked through the git history and it seems to have been that way since the very initial import to GitHub. If we think that we should still suppress the context menu though, I can reimplement it in a React-friendly way, by binding the suppress handler to the root Component of the app.

So, there are two ways forward:

  1. Add our own context menu to the tree nodes, and stop suppressing the browser context menu. Right clicking anywhere in the app that doesn't have a context menu of it's own will open the browser context menu.
  2. Add our own context menu to the tree node and change how we suppress the browser context menu (to do so via a React event handler). Right clicking anywhere in the app that doesn't have a context menu of its own will not open the browser context menu.

I have chosen Option 1 above but can pivot to Option 2 if desired. I think Option 1 is the least surprising behavior to the user.

Why does calling preventDefault in a contextmenu handler on document break the React menu?

TL;DR: React events are synthetic and interact strangely with browser bubbling logic sometimes.

React events are synthetic. If you have a handler defined in oncontextmenu on a component deep in the tree, React creates a binding to contextmenu on the document. Then, it dispatches the event down the component tree itself. As a result, the global handler added in Explorer.tsx to suppress the browser context menu and React's handler are peers, rather than the global handler being defined in a parent element. Critically, this means they will execute in the order they are registered, rather than the normally-expected behavior of executing the deeper handler before the shallower one.

In addition, the Fluent UI handler logic checks event.isDefaultPrevented() before opening the context menu. Normally, this would seem to be OK since the context menu handler should run first (it should be the "deepest" handler). But because our handler is registered on the document, these are peer handlers from the browser's perspective. The global one in Explorer.tsx runs first, prevents the default browser behavior, and thus blocks out the context menu from invoking.

jawelton74
jawelton74 previously approved these changes May 28, 2024
@languy
Copy link
Member

languy commented May 29, 2024

So, there are two ways forward:

  1. Add our own context menu to the tree nodes, and stop suppressing the browser context menu. Right clicking anywhere in the app that doesn't have a context menu of it's own will open the browser context menu.
  2. Add our own context menu to the tree node and change how we suppress the browser context menu (to do so via a React event handler). Right clicking anywhere in the app that doesn't have a context menu of its own will not open the browser context menu.

I have chosen Option 1 above but can pivot to Option 2 if desired. I think Option 1 is the least surprising behavior to the user.

I also vote for Option 1: the context menu has useful functionality. It would be detrimental to the UX to prevent the user from accessing them, IMHO (and personally, I am a fan of the "Inspect" option which I can finally use on DE :)).

Copy link
Contributor

@sevoku sevoku left a comment

Choose a reason for hiding this comment

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

LGTM and +1 for context menu Option 1

@analogrelay analogrelay merged commit 98c5fe6 into master May 29, 2024
20 checks passed
@analogrelay analogrelay deleted the ashleyst/merge-resource-trees branch May 29, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants