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

Adding alert label if file/size limit exceeded in selections table #1183 #1228

Merged
merged 4 commits into from
Apr 25, 2022

Conversation

sam-glendenning
Copy link
Contributor

@sam-glendenning sam-glendenning commented Apr 20, 2022

Description

This implementation is designed to mimic Topcat as much as possible.

Here is Topcat:
160371182-998c26c0-8e54-410b-923e-3589b8358880

And here is DataGateway with this feature:
Capture

Later amendment - this is the updated screenshot
image

Both the text and the label are open to change if we feel its too wordy, etc. Alternatively, if we don't like the design at all, we can have an error notification appear instead. I wasn't too keen on this idea as it may cause notifications to appear too frequently if the user is excessively amending the selections table. I was also more of a fan of the error message appearing next to the offending item so the user can more easily tell what's wrong.

Testing instructions

Best run through SciGateway so the selections can be customised. To make things easier, change the fileCountMax and totalSizeMax values in download settings to much smaller values so that the labels can be displayed more easily. Try adding and removing things from the selections - the labels should appear and disappear as necessary.

  • Review code
  • Check Actions build
  • Review changes to test coverage

Agile board tracking

Closes #1183

This is designed to mimick Topcat as much as possible
@sam-glendenning sam-glendenning added enhancement New feature or request datagateway-download Issues relating to the download plugin user feedback Issues that were raised by users labels Apr 20, 2022
Prevents an empty Grid item from appearing
@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #1228 (d18e614) into develop (3cb1ca2) will increase coverage by 0.00%.
The diff coverage is 81.81%.

@@           Coverage Diff            @@
##           develop    #1228   +/-   ##
========================================
  Coverage    97.51%   97.51%           
========================================
  Files          133      133           
  Lines         6753     6761    +8     
  Branches      2000     2008    +8     
========================================
+ Hits          6585     6593    +8     
  Misses         153      153           
  Partials        15       15           
Flag Coverage Δ
common 98.14% <ø> (ø)
dataview 98.01% <ø> (ø)
download 94.85% <81.81%> (+0.04%) ⬆️
search 97.51% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...d/src/downloadCart/downloadCartTable.component.tsx 93.28% <81.81%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cb1ca2...d18e614. Read the comment docs.

Copy link
Member

@louise-davies louise-davies left a comment

Choose a reason for hiding this comment

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

See above comment. Also, if we're using the Alert component for the other two warnings, should we use this warning for the empty_items_warning? It would make it look more error like and since we've already included the mui-lab library...

Comment on lines 387 to 388
Too many files - you have exceeded limit of {fileCountMax}{' '}
files - please remove some files
Copy link
Member

Choose a reason for hiding this comment

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

These strings need to be in the translation file - you should still be able to parameterise fileCountMax and totalSizeMax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've given this some thought and decided to combine our ideas. I say we provide one <Alert> object, like the one I've done, in the place where your empty_items_warning is so that they aren't too many error messages on the page. We keep that <Alert> there, with the appropriate error message, until no further errors remain on the page.

Empty items warning is now combined with alerts for file and size limits. Alert has also been moved to where the empty items warning was, above the download and remove all buttons.

Also moved text for errors to translation file
Copy link
Member

@louise-davies louise-davies left a comment

Choose a reason for hiding this comment

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

Good that the changes are addressed - I'm just wondering if it's better to only display one error or to display the errors individually. I'm thinking of times when I've been frustrated when filling out a form that only displayed one error meant that I felt I was playing whack-a-mole with the errors vs if you show all the errors individually then the user knows everything they need to know at once.

Although I'm not sure it's too big of a deal, since the error message will update on every action the user does to try and remedy the message (i.e. removing items from the cart via the remove item buttons) so perhaps it's fine to only show a single error? Thoughts?

@sam-glendenning
Copy link
Contributor Author

sam-glendenning commented Apr 22, 2022

Kinda flip flopping on the idea. Maybe it is nicer to put the error message right next to the offending item for convenience (first screenshot). However, if all errors are present, this could cause:

  • error message next to file count
  • error message next to size calculation
  • error message below all them and above download button if empty files present

To top it all off, I presume we'd want to use Typography elements in all error messages as well, which causes a larger height of the entire message, which could push the height of the page down too.

@louise-davies
Copy link
Member

@sam-glendenning true about the height of the page, but I don't mind that too much as it should be a rare occurrence that the user needs to rectify themselves right? The empty item message should be very rare I'd hope, and not occur at the same time as the max items messages often. Also I wouldn't think we'd need a Typography element as the Alert is already a MUI component which expects text right? Main reason to use Typography is to ensure text inherits MUI props/theming etc.

@sam-glendenning
Copy link
Contributor Author

Fair enough. I'll put the alerts back where they were

This means we now have a separate error message for every error
@sam-glendenning
Copy link
Contributor Author

@louise-davies back we go again!

image

@sam-glendenning sam-glendenning merged commit 7cff7ca into develop Apr 25, 2022
@sam-glendenning sam-glendenning deleted the feature/warning-if-limit-exceeded-#1183 branch April 25, 2022 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datagateway-download Issues relating to the download plugin enhancement New feature or request user feedback Issues that were raised by users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display warning to user if they exceed number of file/total size limits
2 participants