Kevin yoo/hmp 157 add error messages to workspace creation dialog 10 datasets protected datasets selected #3215

kevinyooky · 2023-08-04T08:40:32Z

This branch is the main branch for this feature. I have chained other branches from this one to prevent PRs from getting too large.

context/app/static/js/components/workspaces/CreateWorkspaceDialog/CreateWorkspaceDialog.jsx

context/app/static/js/shared-styles/DialogModal/DialogModal.jsx

…s' into kevin-yoo/HMP-157-Add-error-messages-to-workspace-creation-dialog-10-datasets-protected-datasets-selected

kevinyooky · 2023-08-04T23:39:17Z

context/app/static/js/components/workspaces/CreateWorkspaceInput/CreateWorkspaceInput.jsx

The additional space in between the quotes was creating an empty gap in the dialog.

kevinyooky · 2023-08-04T23:40:22Z

context/app/static/js/pages/SavedList/SavedList.jsx

This was to fix the build error which required the provider wrapping around savedList in order to access the store.

The SelectableTableProvider component should be usable in place of the raw Provider and createStore calls: https://github.com/hubmapconsortium/portal-ui/blob/main/context/app/static/js/shared-styles/tables/SelectableTableProvider/SelectableTableProvider.jsx#L6

In general though, I'm pretty sure this should be the same store instance that the selected list is saved in; otherwise, you have two instances of the state that may not necessarily be synchronized.

The instance of the zustand store that the saved entities table uses is created as part of the default export: https://github.com/hubmapconsortium/portal-ui/tree/main/context/app/static/js/components/savedLists/SavedEntitiesTable/SavedEntitiesTable.jsx#L130

I think it would make sense to import the version of SavedEntitiesTable that doesn't have that provider exported with it in this file so that you can guarantee the list and error logic are using the same store instance.

kevinyooky · 2023-08-04T23:40:50Z

context/app/static/js/shared-styles/DialogModal/DialogModal.jsx

@@ -28,6 +30,11 @@ function DialogModal({
        </Typography>
      </StyledDialogTitle>
      <DialogContent>
+        {errorMessage && (
+          <Alert $marginBottom={20} severity="error">


sx prop for marginBottom did not work so passed in marginBottom prop for Alert

Setting the marginBottom works fine here, but I'd like to shed some light on why sx didn't work here and how our upcoming changes will make it work in more places.

The imported Alert component is a styled-component with an explicit $marginBottom prop. The sx prop's style is thus overwritten by the styles defined for that component (if marginBottom is not in the props, then the margin-bottom: 0px style is still applied as it's currently implemented).

https://github.com/hubmapconsortium/portal-ui/tree/main/context/app/static/js/shared-styles/alerts/index.js

Once we convert these sorts of components to be handled via MUI's styled util or via theme, we'll be able to use the sx prop in more places.

That makes sense.

NickAkhmetov

Great progress! A few minor notes on my end.

NickAkhmetov · 2023-08-05T18:54:18Z

context/app/static/js/shared-styles/DialogModal/DialogModal.jsx

@@ -28,6 +30,11 @@ function DialogModal({
        </Typography>
      </StyledDialogTitle>
      <DialogContent>
+        {errorMessage && (
+          <Alert $marginBottom={20} severity="error">


Setting the marginBottom works fine here, but I'd like to shed some light on why sx didn't work here and how our upcoming changes will make it work in more places.

The imported Alert component is a styled-component with an explicit $marginBottom prop. The sx prop's style is thus overwritten by the styles defined for that component (if marginBottom is not in the props, then the margin-bottom: 0px style is still applied as it's currently implemented).

https://github.com/hubmapconsortium/portal-ui/tree/main/context/app/static/js/shared-styles/alerts/index.js

Once we convert these sorts of components to be handled via MUI's styled util or via theme, we'll be able to use the sx prop in more places.

NickAkhmetov · 2023-08-05T19:07:36Z

context/app/static/js/components/workspaces/CreateWorkspaceDialog/CreateWorkspaceDialog.jsx

+          selectedRows.size > 10
+            ? `You have selected ${selectedRows.size} datasets. Workspaces currently only supports up to 10 datasets. Please unselect datasets.`
+            : null


Great job on this change - this should make it a lot easier for us to provide future error messages/feedback in this style.

On a somewhat more optional note: consider defining errorMessage (and other conditional props in the future) as a variable on the line right after you get selectedRows:

const { selectedRows } = useStore(); const errorMessage = selectedRows.size > 10 ? 'you have selected ...' : null;

errorMessage={errorMessage}

This approach helps with readability when ternary statements are longer than a single line.

NickAkhmetov · 2023-08-07T13:49:16Z

context/app/static/js/pages/SavedList/SavedList.jsx

The SelectableTableProvider component should be usable in place of the raw Provider and createStore calls: https://github.com/hubmapconsortium/portal-ui/blob/main/context/app/static/js/shared-styles/tables/SelectableTableProvider/SelectableTableProvider.jsx#L6

In general though, I'm pretty sure this should be the same store instance that the selected list is saved in; otherwise, you have two instances of the state that may not necessarily be synchronized.

The instance of the zustand store that the saved entities table uses is created as part of the default export: https://github.com/hubmapconsortium/portal-ui/tree/main/context/app/static/js/components/savedLists/SavedEntitiesTable/SavedEntitiesTable.jsx#L130

I think it would make sense to import the version of SavedEntitiesTable that doesn't have that provider exported with it in this file so that you can guarantee the list and error logic are using the same store instance.

…space-creation-dialog-10-datasets-protected-datasets-selected

NickAkhmetov · 2023-08-18T16:41:53Z

Updated demos

Create Workspace dialog with both errors visible:

Dismissing the errors reactivates the workspace name field and the launch button:

2023-08-18.workspace.error.workflow.demo.mov

Storybook demo for the four snackbar variants, which can be triggered from anywhere on the site:

2023-08-18.snackbar.story.demo.mov

Selecting a single protected dataset properly handles the non-plural case in the error message:

NickAkhmetov · 2023-08-21T17:11:00Z

TODO: Switch over vitessce visualization toasts from sharing config and from viewing vitessce in firefox

john-conroy

Thanks for picking this up! The only blocking comment is the one made in CreateWorkspaceDialog - everything else you can take or leave.

john-conroy · 2023-08-22T15:04:51Z

context/.storybook/main.js

+    '../app/static/js/shared-styles/**/*.stories.js',
+    '../app/static/js/shared-styles/**/*.stories.tsx',
+    '../app/static/js/components/**/*.stories.js',
+    '../app/static/js/components/**/*.stories.tsx',


Suggested change

'../app/static/js/shared-styles/**/*.stories.js',

'../app/static/js/shared-styles/**/*.stories.tsx',

'../app/static/js/components/**/*.stories.js',

'../app/static/js/components/**/*.stories.tsx',

'../app/static/js/**/*.stories.@(js|jsx|ts|tsx)'

Perhaps this could simplify it?

john-conroy · 2023-08-22T15:07:30Z

context/app/static/js/components/entity-search/MetadataMenu/MetadataMenu.jsx

+  datasets: (rowCount) =>
+    `You have selected ${rowCount} datasets. Workspaces currently only supports up to 10 datasets. Please unselect datasets.`,
+  protectedDataset: (rows) =>
+    `You have selected a protected dataset (${rows[0]._source.hubmap_id}). Workspaces currently only supports published public datasets. To remove the protected dataset from workspace creation, click the “Remove Protected Datasets” button below or return to the previous screen to manually remove this dataset.`,
+  protectedDatasets: (rows) =>
+    `You have selected ${rows.length} protected datasets. Workspaces currently only supports published public datasets. To remove protected datasets from this workspace creation, click the “Remove Protected Datasets” button below or return to the previous screen to manually remove those datasets.`,


Should we just pass rows for consistency? Both datasets and protectedDatasets depend on the length of the rows array.

I decided to pass the size here initially because selectedRows is a set, while protectedRows is an array; I didn't like rows.size in one case and rows.length in the other.

I've applied this revision but modified the arg names and added an explanatory comment to ensure clarity re: why this difference is here.

john-conroy · 2023-08-22T15:10:25Z

context/app/static/js/components/entity-search/MetadataMenu/MetadataMenu.jsx

+  const { selectedRows } = useSelectableTableStore();
+  const protectedRows = useDatasetsAccessLevel(selectedRows.size > 0 ? [...selectedRows] : []).datasets;
+  const containsProtectedDataset = protectedRows.length > 0;
+
+  const errorMessages = [];
+
+  if (selectedRows.size > 10) {
+    errorMessages.push(errorHelper.datasets(selectedRows.size));
+  }
+
+  if (containsProtectedDataset) {
+    if (protectedRows.length === 1) {
+      errorMessages.push(errorHelper.protectedDataset(protectedRows));
+    } else {
+      errorMessages.push(errorHelper.protectedDatasets(protectedRows));
+    }
+  }


We could add a wrapper component around CreateWorkspaceDialog and push all of that logic there.

john-conroy · 2023-08-22T15:12:25Z

context/app/static/js/components/entity-search/SearchWrapper/utils.js

@@ -220,6 +220,8 @@ function createSearchkitFacet({ field, identifier, label, type, ...rest }) {
  });
 }

+const defaultFields = createField({ fieldName: 'mapped_data_access_level', label: 'mapped_data_access_level' });


Do we still need this? I remember Kevin and I introduced this at some point, but later moved towards making a request with useDatasetAccessLevel.

Thanks for the context - I initially left this intact since I wasn't familiar with what this was doing, and have removed it after testing that it doesn't impact functionality.

john-conroy · 2023-08-22T15:15:36Z

context/app/static/js/components/workspaces/CreateWorkspaceDialog/CreateWorkspaceDialog.jsx

+
+  const protectedHubmapIds = protectedRows?.map((row) => row._source.hubmap_id).join(', ');
+  const { toastSuccess } = useSnackbarStore();
+  const { deselectRows } = useSelectableTableStore();
+
+  const removeProtectedDatasets = () => {
+    deselectRows(protectedRows.map((r) => r._id));
+    toastSuccess('Protected datasets successfully removed from selection.');
+  };
+


CreateWorkspaceDialog is used outside of the search page on both the workspaces page and datasets pages. I don't believe this component will have access to the SelectableTableProvider store. We should move this to a component around CreateWorkspaceDialog for use on the search page.

I've addressed this and the MetadataMenu comment accordingly; I've created a CreateWorkspaceWithDatasetsDialog component which wraps the CreateWorkspaceDialog component and performs error checking. The field is then passed in to the CreateWorkspaceDialog via a renderProp which will also allow us to add any other additional fields we may require in the future.

NickAkhmetov · 2023-08-22T15:37:44Z

Following the discussion during yesterday's planning meeting, I have converted the Vitessce snackbars to use the new toast mechanism for consistency. I was not able to trigger the long URL condition for the vitessce copy functionality (to my understanding, modern browsers all support 8k+ URL lengths and this functionality dates back to IE only allowing 2048 characters) but the logic in the code is set to trigger a warning if a long URL is detected.

I've also set the Vitessce Firefox performance warning to only pop up once for each user via a localStorage key. This functionality was particularly annoying on publication pages where it would pop up every time the expanded vignette changed.

2023-08-22.vitessce.snackbar.updates.mov

I'll start work on addressing John's feedback.

…ray distinction

…h datasets" case

john-conroy · 2023-08-29T14:17:01Z

context/app/static/js/shared-styles/snackbars/store.ts

+const useSnackbarActions = () => {
+  return useStore((store) => ({
+    toastError: store.toastError,
+    toastInfo: store.toastInfo,
+    toastWarning: store.toastWarning,
+    toastSuccess: store.toastSuccess,
+    closeSnackbar: store.closeSnackbar,
+  }));
+};


I lean towards not selecting the entire state by default as each component will update when any of the variables change even if the component does not depend on them. We could implement something like https://docs.pmnd.rs/zustand/guides/auto-generating-selectors if writing selectors ourselves is too much boilerplate.

Thanks for the heads up! I agree completely about not selecting the entire state (since that would be more or less identical to using context). The auto generating selectors functionality seems very useful, and would be a solid improvement for us to make once we're on zustand v4.

TL;DR: - I can take this helper out and replace it with specific selectors where needed, but I'm not sure how much benefit that would bring in this case since these are just functions.

To my understanding, since the keys accessed/returned by this helper are functions, they shouldn't be updating at all; the idea was that this helper would let us skip making a separate selector for each case where we just need to be able to send a snackbar message without necessarily needing to read it. Additionally, if zustand functions are regenerated on each state update, wouldn't they all be regenerated regardless of whether they're accessed separately or all together in a helper like this?

john-conroy · 2023-08-29T14:19:55Z

context/app/static/js/components/detailPage/visualization/Visualization/Visualization.jsx

+  // Show a warning if the user is using Firefox.
+  useEffect(() => {
+    if (sniffBrowser() === 'Firefox' && !localStorage.getItem(localStorageFirefoxWarningKey)) {
+      toastError(FIREFOX_WARNING);
+      localStorage.setItem(localStorageFirefoxWarningKey, true);
+    }
+  }, [toastError]);
+
+  // Show instructions for exiting full screen mode when the user enters full screen mode.
+  useEffect(() => {
+    if (vizIsFullscreen) {
+      toastInfo('Press [esc] to exit full window.');
+    }
+  }, [vizIsFullscreen, toastInfo]);


I think I prefer to pull related hooks together into a single hook, but it's entirely a preference. Do you prefer to have everything in a single file?

It's on a case-by-case basis for me, but I lean towards keeping hooks focused on handling a single functionality/behavior:

If there's an interdependent process where separating it leads to the output of one function being fed into another function, I do like to unify those into one on at least some level (e.g. if the SWR output from one fetch is used to set the params of a second fetch, it's helpful to have a single hook which abstracts that detail out from the component, but still keep the other two SWR calls as their own hooks)

If it's two different conditions that trigger similar effects, I still keep them separated in order to keep the code maintainable and prevent unnecessary computations

RE: maintainable - as time goes on, combining more and more business logic into one hook/into one function leads to big fragile monoliths where

RE: unnecessary computations - in this case, combining these two useEffects into one would lead to extra sniffBrowser executions/potential extra localStorage reads whenever the visualization is full-screened; these are trivial operations, but in other circumstances these could be e.g. lookups from an API.

…space-creation-dialog-10-datasets-protected-datasets-selected

NickAkhmetov · 2023-09-13T16:51:14Z

Note: the fixes in this branch coincide with the changes in #3258 as I initially didn't realize the lint issues came from upstream. If this branch is merged in, we can close that PR.

…space-creation-dialog-10-datasets-protected-datasets-selected

kevinyooky added 4 commits August 4, 2023 00:56

workspace dialog styling

28c41b7

get count of selected datasets

34b45e8

alert styling and modal styling

c89617a

dialog styling

75fff98

kevinyooky requested review from NickAkhmetov, john-conroy, lchoy and tsliaw August 4, 2023 08:40

changelog

7b79d27

kevinyooky commented Aug 4, 2023

View reviewed changes

context/app/static/js/components/workspaces/CreateWorkspaceDialog/CreateWorkspaceDialog.jsx Outdated Show resolved Hide resolved

NickAkhmetov reviewed Aug 4, 2023

View reviewed changes

context/app/static/js/components/workspaces/CreateWorkspaceDialog/CreateWorkspaceDialog.jsx Outdated Show resolved Hide resolved

context/app/static/js/shared-styles/DialogModal/DialogModal.jsx Outdated Show resolved Hide resolved

kevinyooky added 13 commits August 4, 2023 11:25

use mui Box

5deb66d

spacing for alert using mui Box

e99dee2

remove uneccesary emtpy line

798993c

implement provider for zustand store

6644e86

remove semicolon

899b249

cleanup

936c0a8

cleanup

65a7ad2

Merge branch 'kevin-yoo/implement-error-message-for-protected-dataset…

aa1fa0e

…s' into kevin-yoo/HMP-157-Add-error-messages-to-workspace-creation-dialog-10-datasets-protected-datasets-selected

build error

8d05d85

margin bottom for alert with box

6069f5a

remove unused component

1dffa49

refactor DialogModal to take in error prop

bf740c1

remove unused

5ce58c7

kevinyooky commented Aug 4, 2023

View reviewed changes

kevinyooky requested a review from NickAkhmetov August 4, 2023 23:45

NickAkhmetov reviewed Aug 7, 2023

View reviewed changes

kevinyooky added 2 commits August 7, 2023 09:18

git undo build

a499fe4

Merge branch 'main' into kevin-yoo/HMP-157-Add-error-messages-to-work…

c823458

…space-creation-dialog-10-datasets-protected-datasets-selected

Remove redundant wrapper

b4d3c03

HMP-333 Persist selections on dataset test-search page (#3230)

e706d8c

john-conroy requested changes Aug 22, 2023

View reviewed changes

NickAkhmetov added 3 commits August 22, 2023 11:26

Convert vitessce notifications to use new snackbars

89e1b2e

Don't display firefox warning if it's already been seen

4f91da4

Make sure all actions are available via useSnackbarActions helper hook

1ed24e7

NickAkhmetov added 4 commits August 22, 2023 11:40

simplify stories matcher in config

59fd9eb

always pass rows, but specify selected rows vs. protected rows set/ar…

89decb8

…ray distinction

Use component composition/renderProps to handle "create workspace wit…

4a9cfc7

…h datasets" case

wrap renderprops in useCallback since they're being passed to a child

9d83d28

NickAkhmetov mentioned this pull request Aug 23, 2023

Nickakhmetov/HMP-339 data products section #3235

Merged

NickAkhmetov requested a review from john-conroy August 24, 2023 00:04

john-conroy reviewed Aug 29, 2023

View reviewed changes

NickAkhmetov requested a review from john-conroy August 31, 2023 17:05

john-conroy previously approved these changes Sep 11, 2023

View reviewed changes

Merge branch 'main' into kevin-yoo/HMP-157-Add-error-messages-to-work…

8f3ff79

…space-creation-dialog-10-datasets-protected-datasets-selected

NickAkhmetov dismissed john-conroy’s stale review via 8f3ff79 September 13, 2023 15:35

lint fixes from upstream

3440e6e

Merge branch 'main' into kevin-yoo/HMP-157-Add-error-messages-to-work…

ffbe845

…space-creation-dialog-10-datasets-protected-datasets-selected

NickAkhmetov mentioned this pull request Sep 13, 2023

Nickakhmetov/HMP-348 Dependency organization/cleanup/updates #3259

Merged

john-conroy previously approved these changes Sep 19, 2023

View reviewed changes

Merge branch 'main' into kevin-yoo/HMP-157-Add-error-messages-to-work…

04546ae

…space-creation-dialog-10-datasets-protected-datasets-selected

NickAkhmetov dismissed john-conroy’s stale review via 04546ae September 19, 2023 16:48

NickAkhmetov approved these changes Sep 19, 2023

View reviewed changes

NickAkhmetov merged commit 98a59f5 into main Sep 19, 2023
6 of 7 checks passed

NickAkhmetov deleted the kevin-yoo/HMP-157-Add-error-messages-to-workspace-creation-dialog-10-datasets-protected-datasets-selected branch September 19, 2023 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kevin yoo/hmp 157 add error messages to workspace creation dialog 10 datasets protected datasets selected #3215

Kevin yoo/hmp 157 add error messages to workspace creation dialog 10 datasets protected datasets selected #3215

kevinyooky commented Aug 4, 2023 •

edited

Loading

kevinyooky Aug 4, 2023 •

edited

Loading

kevinyooky Aug 4, 2023

NickAkhmetov Aug 7, 2023

kevinyooky Aug 4, 2023 •

edited

Loading

NickAkhmetov Aug 5, 2023

kevinyooky Aug 7, 2023

NickAkhmetov left a comment

NickAkhmetov Aug 5, 2023

NickAkhmetov Aug 5, 2023

NickAkhmetov Aug 7, 2023

NickAkhmetov commented Aug 18, 2023

NickAkhmetov commented Aug 21, 2023

john-conroy left a comment

john-conroy Aug 22, 2023

NickAkhmetov Aug 22, 2023

john-conroy Aug 22, 2023

NickAkhmetov Aug 22, 2023

john-conroy Aug 22, 2023

john-conroy Aug 22, 2023

NickAkhmetov Aug 22, 2023

john-conroy Aug 22, 2023

NickAkhmetov Aug 22, 2023 •

edited

Loading

NickAkhmetov commented Aug 22, 2023

john-conroy Aug 29, 2023

NickAkhmetov Aug 29, 2023

john-conroy Aug 29, 2023

NickAkhmetov Aug 29, 2023

NickAkhmetov commented Sep 13, 2023

Kevin yoo/hmp 157 add error messages to workspace creation dialog 10 datasets protected datasets selected #3215

Kevin yoo/hmp 157 add error messages to workspace creation dialog 10 datasets protected datasets selected #3215

Conversation

kevinyooky commented Aug 4, 2023 • edited Loading

kevinyooky Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinyooky Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickAkhmetov 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

NickAkhmetov commented Aug 18, 2023

Updated demos

NickAkhmetov commented Aug 21, 2023

john-conroy 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

NickAkhmetov Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

NickAkhmetov commented Aug 22, 2023

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

NickAkhmetov commented Sep 13, 2023

kevinyooky commented Aug 4, 2023 •

edited

Loading

kevinyooky Aug 4, 2023 •

edited

Loading

kevinyooky Aug 4, 2023 •

edited

Loading

NickAkhmetov Aug 22, 2023 •

edited

Loading