Use wildcard popup for confirmation #33637

unclejustin · 2022-04-08T01:06:21Z

This PR replaces the built-in window.confirm dialogs with branded modals.

It does this by adding three new components.

Shared ConfirmationModal: Generic modal to provide consistent formatting and functionality
ConfirmDeleteModal: Implementation of ConfirmationModal with delete specific text and handlers
ConfirmRemoveModal: Implementation of ConfirmationModal with remove specific text and handlers

I noticed that all of the call outs to delete and remove could be co-located with the actual menu items, which cleaned up the implementation quite a bit. Now context menu just triggers these modals which then handle dispatching the requests.

Test plan

Navigate to a dashboard with insights
Use the insight context menu and select "Delete"
Should see new confirmation modal
Modal should be cancelable
Modal should trigger deletion
Repeat above for remove

App preview:

Link

vovakulikov

@unclejustin I left a few comments about general things around components and one thing that I would change before merging this PR but it perhaps requires @AlicjaSuska input.

vovakulikov · 2022-04-08T07:46:03Z

client/shared/src/components/ConfirmationModal.tsx

@@ -0,0 +1,30 @@
+import React from 'react'


I think we shouldn't populate the "shared" package with new components. If you see value in the ConfirmationModal component, I suggest proposing it in the Wildcard package.

Personal (obvious) note, primarily for myself to wrap my head around: I've recently revisited this paradigm in my head, and I think wildcard should consist only of low-level components (let's say "atoms"). And it is up to consumers to build the right layout/component ("organisms"). In case we see some values in some consumer component that could be reused in different parts of applications and it consists of other atoms and it doesn't have (or could have no) consumer-specific logic then we could extract this logic into wildcard (and call it "molecule")

So the hierarchy is the following

atoms (low-level components, icons, buttons, labels, inputs ...etc.) are stored in the wildcard

molecules (everything that is built with atoms and has nothing about a specific use-case) are stored in the wildcard as well

organisms - combinations of atoms, molecules, specific consumer logic

Unfortunately, we don't have any hierarchy at the moment in the wildcard, but the bottom line is that I think this modal could be part of the wildcard as a "molecule" component. cc @sourcegraph/frontend-platform

I also see other groups of components that stands outside of this system, something like our context or branch pickers. These components are stateful they have fetching logic and their own state inside, so it something that shouldn'd be inside wildcard component. I would say idea of having shared component has some pros for easy reusing this component in different consumers

For example, in code insights we want to reuse context picker in the dilldown panel. So I would say it makes sense to have let's say @sourcegraph/pickers package. cc @sourcegraph/frontend-devs

<ContextPicker contextType={ContextType.query} autofocus={true} // other ContextPicker specific props className={styles.drilldownContextPicker} />

Figma Designs

I heartily agree with all of the above. I will update accordingly.

vovakulikov · 2022-04-08T07:54:20Z

client/shared/src/components/ConfirmationModal.tsx

+    showModal: boolean
+    handleCancel: () => void
+    handleConfirmation: () => void
+    header: React.ReactElement | string


Maybe I was affectet by looking too much at compounds component but I belive it would be better to have styled components around confimration modal header and messge field. Instead of having them as prop here.

For example instead of this (the foollowing component is taken from this PR changes, further on this changes page)

// the current versions <ConfirmationModal showModal={showModal} handleCancel={handleCancel} handleConfirmation={() => handleDelete(insight)} header="Delete Insight?" message={ <> Are you sure you want to delete the insight <strong>{insight.title}</strong>? </>} /> // the way that I propose <ConfirmationModal showModal={showModal} onConfirm={() => handleDelete(insight) onCancel={handleCancel}> <h3>Delete Insight?</h3> <p>Are you sure you want to delete the insight <strong>{insight.title}</strong>?</p> </ConfirmationModal>

This is totally fair. I was perhaps being too rigid in my formatting. I agree with moving all of the content into children.

vovakulikov · 2022-04-08T08:10:51Z

client/shared/src/components/ConfirmationModal.tsx

+    const { showModal, handleCancel, handleConfirmation, header, message } = props
+
+    return (
+        <Modal isOpen={showModal} position="center" aria-label="Delete confirmation modal">


@unclejustin I think aria-label="Delete confirmation modal" is too specific for this since other consumer might have used this in different scenarios.

Womp! That' was a mistake 😅 fixing.

vovakulikov · 2022-04-08T08:13:05Z

client/shared/src/components/ConfirmationModal.tsx

+
+export interface ConfirmationModalProps {
+    showModal: boolean
+    handleCancel: () => void


@unclejustin I remember that we had a discussion (probably with @limitedmage) somewhere in Slack that it would be cool to have some naming convention around our prop callback naming. The rule that I'm trying to stick is that all public callbacks prop are named with on<specic callback action> and all internal handlers inside component are called with handle<action name> names.

Solid! Will do.

vovakulikov · 2022-04-08T08:37:45Z

...erprise/insights/components/insights-view-grid/components/backend-insight/BackendInsight.tsx

@@ -84,7 +84,7 @@ export const BackendInsightView: React.FunctionComponent<BackendInsightProps> =

    // Handle insight delete and remove actions
    const { loading: isDeleting, delete: handleDelete } = useDeleteInsight()
-    const { remove: handleRemove, loading: isRemoving } = useRemoveInsightFromDashboard()
+    const { loading: isRemoving } = useRemoveInsightFromDashboard()


@unclejustin I think I don't fully understand this hook here. useRemoveInsightFromDashboard() hook here exposes loading state variable (inside of useRemoveInsightFromDashboard loading is just an internal state that created with useState hook)

And we do not call here any delete logic (callback) from this hook, therefore this loading state will never be in "true" position. Therefore our UI about deleting/removing insight inside of insight card will never be rendered.

It's actually a good moment for us to revisit UI about removing and deleting insight. @AlicjaSuska maybe instead of having loading state inside insight card we could have loading state inside "delete insight" confirmation modal and close this modal if an insight was sucessfully deleted and show error inside confirmaton model if it wasn't

Ah dang, you are totally correct! I was expecting that loading would be shared across components. I was wrong. So very wrong #shame.

I'm going to move the "loading" UI into the modal for now and see what Alicja thinks of it.

vovakulikov · 2022-04-08T08:39:59Z

...nsights/components/insights-view-grid/components/insight-context-menu/ConfirmDeleteModal.tsx

+    showModal,
+    handleCancel,
+}) => {
+    const { delete: handleDelete } = useDeleteInsight()


So here we could use loading state from useDeleteInsight and for example render loading spinner next to submit button inside confirmation modal and disable closing this model by esc or outside click. @AlicjaSuska I think this might be a good compomise while we don't have a good way to notify user that we've done something (I remember that we had a convo about notification panel for actions like this)

Using loading here now as suggested. Just disabling the buttons and closing for now. Will sync up with Alicja on Monday.

...nsights/components/insights-view-grid/components/insight-context-menu/InsightContextMenu.tsx

unclejustin · 2022-04-08T22:42:17Z

@vovakulikov I've addressed all the issues I can without getting feedback from @AlicjaSuska . Alicja I will try and sync up with you next week to figure out the rest of these issues.

Vova, no further work is needed from you until the design issues are resolved. Thank you for such a thorough review!

sourcegraph-bot · 2022-04-08T22:42:50Z

Codenotify: Notifying subscribers in OWNERS files for diff 61b9633155ae530a288cfe7e08bf25198da8fa72...77e40997713400c568606815fa42fa3dcfcd7093.

No notifications.

unclejustin · 2022-04-11T23:40:17Z

@vovakulikov updated with design tweaks from Alicja. Please re-review 🙏

Note: this still needs a storybook entry before it can be merged in, but I want to wait on that until it passes tech review. Assuming you approve I will add a story for the modals and ping frontend platform team for a final review.

AlicjaSuska · 2022-04-13T08:04:32Z

@unclejustin @vovakulikov I left a comment about the copy in the main issue

unclejustin · 2022-04-13T15:41:00Z

@AlicjaSuska your comments have been implemented thank you! 🙇

@vovakulikov ready for code review

@sourcegraph/frontend-platform I'm proposing adding a component in this PR. Could you kindly review? If you think this should be moved to its own separate PR please let me know ASAP. I'll change this one to be self contained and open up another. Thanks!

vovakulikov

@unclejustin good work, I left a few small comments but besides this it looks great!

vovakulikov · 2022-04-13T16:25:53Z

...erprise/insights/components/insights-view-grid/components/backend-insight/BackendInsight.tsx

@@ -162,8 +158,6 @@ export const BackendInsightView: React.FunctionComponent<BackendInsightProps> =
                            menuButtonClassName="ml-1 d-inline-flex"
                            zeroYAxisMin={zeroYAxisMin}
                            onToggleZeroYAxisMin={() => setZeroYAxisMin(!zeroYAxisMin)}
-                            onRemoveFromDashboard={dashboard => handleRemove({ insight, dashboard })}


Nice! I really like how everything is simplified now here.

...nsights/components/insights-view-grid/components/insight-context-menu/InsightContextMenu.tsx

vovakulikov · 2022-04-13T16:41:07Z

client/web/src/enterprise/insights/hooks/use-remove-insight.ts

                return
            }

            setLoading(true)
            setError(undefined)

            try {
+                await sleep(5000)


Do we need this? or is this something that sneaked to this PR during testing? @unclejustin

YIKES NO

Thank you for catching this embarrassing debugging code 😅

client/wildcard/src/components/ConfirmationModal/ConfirmationModal.tsx

Move ConfirmationModal to wildcard. Move loading check into modals.

cla-bot bot added the cla-signed label Apr 8, 2022

unclejustin requested a review from vovakulikov April 8, 2022 01:20

vovakulikov reviewed Apr 8, 2022

View reviewed changes

unclejustin mentioned this pull request Apr 8, 2022

Move InsightContextMenu permissions checks into features UI #33691

Open

unclejustin force-pushed the 32374-code-insights-use-wildcard-popup-instead-of-native-confirmation-modal-for-insight-operation-confirmation-step branch from fce5d74 to 75f73b9 Compare April 11, 2022 23:38

unclejustin requested a review from vovakulikov April 11, 2022 23:38

unclejustin force-pushed the 32374-code-insights-use-wildcard-popup-instead-of-native-confirmation-modal-for-insight-operation-confirmation-step branch from 75f73b9 to 546b36e Compare April 13, 2022 15:32

unclejustin requested a review from a team April 13, 2022 15:41

vovakulikov approved these changes Apr 13, 2022

View reviewed changes

unclejustin force-pushed the 32374-code-insights-use-wildcard-popup-instead-of-native-confirmation-modal-for-insight-operation-confirmation-step branch from 84343b7 to c4e0365 Compare April 19, 2022 16:10

unclejustin enabled auto-merge (squash) April 19, 2022 16:59

Justin Boyson added 14 commits April 20, 2022 12:14

Add ConfirmationModal shared component

95d581e

update use-delete-insight to use ConfirmationModal

5792865

update remove from dashboard

cc0a32f

move handle delete into context menu

4a0bff9

refactor confirmation modals

c70267c

remove unused modal

aac1887

review updates

1bc8833

Move ConfirmationModal to wildcard. Move loading check into modals.

design updates

a02ef5e

add cancel and confirm params

0da6408

match text to designs

0a510fc

remove sleep 😬

40c3fc4

fix rebase conflicts

c3379d6

move ConfirmationModal out of Wildcard

73d7b82

add changelog

4d78f53

Justin Boyson added 3 commits April 20, 2022 12:14

fix ts linting error

4038ee0

fix failing test

13e9b15

remove unused functions

a64d5b5

unclejustin force-pushed the 32374-code-insights-use-wildcard-popup-instead-of-native-confirmation-modal-for-insight-operation-confirmation-step branch from 77e4099 to a64d5b5 Compare April 20, 2022 17:14

unclejustin merged commit 2ad489a into main Apr 20, 2022

unclejustin deleted the 32374-code-insights-use-wildcard-popup-instead-of-native-confirmation-modal-for-insight-operation-confirmation-step branch April 20, 2022 17:26

Joelkw mentioned this pull request Apr 27, 2022

insights: note how many dashboards an insight appears on in the delete modal #34601

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use wildcard popup for confirmation #33637

Use wildcard popup for confirmation #33637

unclejustin commented Apr 8, 2022 •

edited by sourcegraph-bot

vovakulikov left a comment

vovakulikov Apr 8, 2022

vovakulikov Apr 8, 2022

unclejustin Apr 8, 2022

vovakulikov Apr 8, 2022

unclejustin Apr 8, 2022

vovakulikov Apr 8, 2022

unclejustin Apr 8, 2022

vovakulikov Apr 8, 2022

unclejustin Apr 8, 2022

vovakulikov Apr 8, 2022

unclejustin Apr 8, 2022

vovakulikov Apr 8, 2022

unclejustin Apr 8, 2022

unclejustin commented Apr 8, 2022

sourcegraph-bot commented Apr 8, 2022 •

edited

unclejustin commented Apr 11, 2022

AlicjaSuska commented Apr 13, 2022

unclejustin commented Apr 13, 2022

vovakulikov left a comment

vovakulikov Apr 13, 2022

vovakulikov Apr 13, 2022

unclejustin Apr 13, 2022

Use wildcard popup for confirmation #33637

Use wildcard popup for confirmation #33637

Conversation

unclejustin commented Apr 8, 2022 • edited by sourcegraph-bot

Test plan

App preview:

vovakulikov left a 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

unclejustin commented Apr 8, 2022

sourcegraph-bot commented Apr 8, 2022 • edited

unclejustin commented Apr 11, 2022

AlicjaSuska commented Apr 13, 2022

unclejustin commented Apr 13, 2022

vovakulikov left a 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

unclejustin commented Apr 8, 2022 •

edited by sourcegraph-bot

sourcegraph-bot commented Apr 8, 2022 •

edited