fix(ui) Fix UI flickering when switching between glossary entities #7432

chriscollins3456 · 2023-02-24T20:56:18Z

Fixes the whole page flickering we would see when switching between terms and nodes in the business glossary. This happened because each entity page itself rendered its own version of the lest side bar browse and the top parent nodes navigation. Whenever you switched to a new glossary entity, it would un-mount and re-mount the whole page causing the whole page to flicker. It also caused an excess of queries such as getting all root terms and root nodes on every change between glossary entities. It also refreshed your sidebar nav to close everything that's open which is a bad experience.

This PR pulls those components out and puts them in a new GlossaryRoutes component that wraps the term and node profiles with them as well as the root glossary page. These components are now fixed!

The tough part is that these components rely on information retrieved from the entity profile component such as the parent nodes and what the current entity is (to know whether or not to highlight it on the sidebar and which parent nodes to default open when you land on a page). There's a few ways we could tackle this, but the simplest and easiest way is to share entityData from the EntityProfile with a new higher GlossaryEntityContext which the two nav components can access. Whenever entityData changes and we're in a glossary context, we update the glossary context with new entity data.

Finally, when we make edits (edit name, move a node/term, delete a node/term, create a node/term) the sidebar browser needs to be aware of those updates. What I implemented was passing in updatedUrns to the glossary context and have the components in that sidebar listen for those changing and refetch and remove themselves from updatedUrns if need be. That way the sidebar component is always up to date with any mutation the user makes.

Here's what it looks like now
https://user-images.githubusercontent.com/28656603/221290287-ed9ed1e2-6380-46f8-ab71-58c2582b633e.mov

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

jjoyce0510 · 2023-02-28T23:17:26Z

datahub-web-react/src/app/entity/EntityRegistry.tsx

@@ -38,6 +39,14 @@ export default class EntityRegistry {
        return this.entities;
    }

+    getNonGlossaryEntities(): Array<Entity<any>> {


jjoyce0510 · 2023-02-28T23:19:49Z

datahub-web-react/src/app/entity/shared/EntityDropdown/CreateGlossaryEntityModal.tsx

@@ -77,6 +80,10 @@ function CreateGlossaryEntityModal(props: Props) {
                        duration: 2,
                    });
                    refetch();
+                    if (isInGlossaryContext) {
+                        const parentNodeToUpdate = getParentNodeToUpdate(entityData, entityType);


Interesting.. So this is only the case when we are editing a glossary term in the context of the glossary... cool

jjoyce0510 · 2023-02-28T23:24:16Z

datahub-web-react/src/app/entity/shared/GlossaryEntityContext.tsx

+    isInGlossaryContext: boolean;
+    entityData: GenericEntityProperties | null;
+    setEntityData: (entityData: GenericEntityProperties | null) => void;
+    updatedUrns: string[];


These fields are less intuitive to understand than the others ... might be helpful to have comment

jjoyce0510 · 2023-02-28T23:27:17Z

datahub-web-react/src/app/entity/shared/containers/profile/EntityProfile.tsx

@@ -303,8 +298,7 @@ export const EntityProfile = <T, U>({
            <>
                <OnboardingTour stepIds={stepIds} />
                <EntityHead />
-                {customNavBar}
-                {showBrowseBar && !customNavBar && <EntityProfileNavBar urn={urn} entityType={entityType} />}
+                {showBrowseBar && <EntityProfileNavBar urn={urn} entityType={entityType} />}


nice kinda like how we are removing custom browse bar

jjoyce0510 · 2023-02-28T23:32:23Z

datahub-web-react/src/app/entity/shared/containers/profile/header/EntityName.tsx

@@ -27,6 +29,7 @@ function EntityName(props: Props) {
    const { isNameEditable } = props;
    const refetch = useRefetch();
    const entityRegistry = useEntityRegistry();
+    const { isInGlossaryContext, updatedUrns, setUpdatedUrns } = useGlossaryEntityData();


This is a bit unfortunate but I see why we have to do it. To communicate this information...

yeah :/ just the tough thing with having the same info displayed in separate components with different fetching happening in different places.

aditya-radhakrishnan · 2023-02-28T23:31:11Z

datahub-web-react/src/app/entity/shared/EntityDropdown/MoveGlossaryEntityModal.tsx

+                    if (isInGlossaryContext) {
+                        const oldParentToUpdate = getParentNodeToUpdate(entityData, entityType);
+                        const newParentToUpdate = selectedParentUrn || getGlossaryRootToUpdate(entityType);
+                        updateGlossarySidebar([oldParentToUpdate, newParentToUpdate], updatedUrns, setUpdatedUrns);
                    }
                }, 2000);


Ah so are we still doing the 2 second wait?

for now unfortunately..

aditya-radhakrishnan · 2023-02-28T23:31:22Z

datahub-web-react/src/app/entity/shared/GlossaryEntityContext.tsx

+    setUpdatedUrns: (updatdUrns: string[]) => void;
+}
+
+export const GlossaryEntityContext = React.createContext<GlossaryEntityContextType>({


I love this solution!

aditya-radhakrishnan · 2023-02-28T23:31:27Z

datahub-web-react/src/app/entity/shared/GlossaryEntityContext.tsx

+    isInGlossaryContext: boolean;
+    entityData: GenericEntityProperties | null;
+    setEntityData: (entityData: GenericEntityProperties | null) => void;
+    updatedUrns: string[];


aditya-radhakrishnan · 2023-02-28T23:31:43Z

datahub-web-react/src/app/entity/shared/constants.ts

@@ -76,6 +76,8 @@ export const ENTITY_TYPES_WITH_MANUAL_LINEAGE = new Set([
    EntityType.DataJob,
 ]);

+export const GLOSSARY_ENTITY_TYPES = [EntityType.GlossaryTerm, EntityType.GlossaryNode];


Nice! Very useful

aditya-radhakrishnan · 2023-02-28T23:32:27Z

datahub-web-react/src/app/entity/shared/containers/profile/utils.ts

+    const previousEntityData = usePrevious(entityData);
+
+    useEffect(() => {
+        // first check this is a glossary entity to prevent unnecessary comparisons in non-glossary context


Thanks for the comment

jjoyce0510 · 2023-02-28T23:33:54Z

datahub-web-react/src/app/entity/shared/containers/profile/utils.ts

+    useEffect(() => {
+        // first check this is a glossary entity to prevent unnecessary comparisons in non-glossary context
+        if (GLOSSARY_ENTITY_TYPES.includes(entityType) && !isEqual(entityData, previousEntityData)) {
+            setEntityData(entityData);


jjoyce0510 · 2023-02-28T23:37:33Z

datahub-web-react/src/app/glossary/utils.ts

+    updatedUrns: string[],
+    setUpdatedUrns: (updatdUrns: string[]) => void,
+) {
+    setUpdatedUrns([...updatedUrns, ...parentNodesToUpdate]);


it's a bit confusing how we are merging the updatedUrns themselves with the parent Node urns... Might be warranting of an explanation via comment

totally. i think the name change rec from below will help with this confusion as well

jjoyce0510 · 2023-02-28T23:40:36Z

datahub-web-react/src/app/glossary/GlossaryRoutes.tsx

+    flex: 1;
+`;
+
+export default function GlossaryRoutes() {


Nice & tight!

jjoyce0510 · 2023-02-28T23:41:58Z

datahub-web-react/src/app/glossary/GlossaryBrowser/NodeItem.tsx

+    useEffect(() => {
+        if (updatedUrns.includes(node.urn)) {
+            refetch();
+            setUpdatedUrns(updatedUrns.filter((urn) => urn !== node.urn));


So after an update the leaf component is responsible for removing itself from the updated Urns list?

Minor: but maybe we consider calling this urnsToUpdate to make the transaction that's happening here clearer

Also, is there any risk in having a single array here?

Instead of urnsToUpdate and updatedUrns as separate structures

ooo i definitely like having urnsToUpdate as a name much more here. and yeah having a singular array i think is fine as long as we're smart about only updating state when se need to (like after the entity that needs to update refetches and only if it's already in the array).

Will update the name!

jjoyce0510 · 2023-02-28T23:43:10Z

datahub-web-react/src/app/glossary/GlossaryBrowser/GlossaryBrowser.tsx

@@ -59,6 +63,17 @@ function GlossaryBrowser(props: Props) {
        }
    }, [refreshBrowser, refetchNodes, refetchTerms]);

+    useEffect(() => {
+        if (updatedUrns.includes(ROOT_NODES)) {


Special case handling for rootNodes and rootTerms? (Where there is no parent) Got it. Might be worth adding a comment somewhere

definitely agreed. a few comments around this change in general wouldn't hurt.

jjoyce0510 · 2023-02-28T23:45:16Z

datahub-web-react/src/app/glossary/utils.ts

+export const ROOT_NODES = 'rootNodes';
+export const ROOT_TERMS = 'rootTerms';
+
+export function getGlossaryRootToUpdate(entityType: EntityType) {


Interesting how we have the special case values in this array. I didn't piece this together until i read all the code and took a step back. Just FYI

fix(ui) Fix UI flickering when switching between glossary entities

0362cdf

chriscollins3456 requested a review from jjoyce0510 February 24, 2023 20:56

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Feb 24, 2023

chriscollins3456 requested a review from aditya-radhakrishnan February 27, 2023 19:39

jjoyce0510 assigned aditya-radhakrishnan Feb 28, 2023

jjoyce0510 reviewed Feb 28, 2023

View reviewed changes

aditya-radhakrishnan approved these changes Feb 28, 2023

View reviewed changes

jjoyce0510 reviewed Feb 28, 2023

View reviewed changes

datahub-web-react/src/app/glossary/GlossaryRoutes.tsx

flex: 1;

`;

export default function GlossaryRoutes() {

Copy link

Collaborator

jjoyce0510 Feb 28, 2023

Choose a reason for hiding this comment

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

Nice & tight!

jjoyce0510 reviewed Feb 28, 2023

View reviewed changes

chriscollins3456 added 2 commits March 5, 2023 16:47

add comments and update state name

17c5704

Merge branch 'master' into cc--fix-glossary-flickering

b999cfa

chriscollins3456 merged commit 4f78de6 into datahub-project:master Mar 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ui) Fix UI flickering when switching between glossary entities #7432

fix(ui) Fix UI flickering when switching between glossary entities #7432

chriscollins3456 commented Feb 24, 2023

jjoyce0510 Feb 28, 2023

jjoyce0510 Feb 28, 2023

jjoyce0510 Feb 28, 2023

aditya-radhakrishnan Feb 28, 2023

jjoyce0510 Feb 28, 2023

jjoyce0510 Feb 28, 2023

chriscollins3456 Mar 5, 2023

aditya-radhakrishnan Feb 28, 2023

chriscollins3456 Mar 5, 2023

aditya-radhakrishnan Feb 28, 2023

aditya-radhakrishnan Feb 28, 2023

aditya-radhakrishnan Feb 28, 2023

aditya-radhakrishnan Feb 28, 2023

jjoyce0510 Feb 28, 2023

jjoyce0510 Feb 28, 2023

chriscollins3456 Mar 5, 2023

jjoyce0510 Feb 28, 2023

jjoyce0510 Feb 28, 2023

jjoyce0510 Feb 28, 2023

chriscollins3456 Mar 5, 2023

jjoyce0510 Feb 28, 2023

chriscollins3456 Mar 5, 2023

jjoyce0510 Feb 28, 2023

fix(ui) Fix UI flickering when switching between glossary entities #7432

fix(ui) Fix UI flickering when switching between glossary entities #7432

Conversation

chriscollins3456 commented Feb 24, 2023

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment