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

EDSC-2895: Fix download granule dropdown in table view #1241

Merged
merged 6 commits into from
Oct 9, 2020
Merged

EDSC-2895: Fix download granule dropdown in table view #1241

merged 6 commits into from
Oct 9, 2020

Conversation

saurabhdaware
Copy link
Contributor

@saurabhdaware saurabhdaware commented Oct 5, 2020

Overview

What is the feature?

Fixes #1227

What is the Solution?

Not 100% sure of the possible cause but it was possibly because rowClick event of table triggering after the onClick of download button so it was immediately removing the dropdown menu.

Edit: Changed implementation to use e.stopPropogation()

What areas of the application does this impact?

It will impact the parts which has the same onRowClick function reference that I've made changes into.
It checks if it has the granule-results-data-links-button__button class to check if it is the download button element.

Testing

Reproduction steps

  • Environment for testing:
  • Collection to test with:
  1. Search for a collection that has multi-file granules and click on it to load granules (NSIDC-0481 in
  2. Click on the download icon in the list view and observe that multiple files appear
  3. Confirm that clicking on those links downloads the intended file
  4. Switch to table view using the toggle at the top of the granules panel
  5. Click on the download icon in the table view and observe that multiple files appear
  6. Confirm that clicking on those links downloads the intended file

Attachments

Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.

Checklist

  • I have added automated tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@saurabhdaware saurabhdaware marked this pull request as draft October 5, 2020 15:08
@abbottry abbottry changed the title add isDownloadClicked condition to rowClick EDSC-2895: Add isDownloadClicked condition to rowClick Oct 5, 2020
@saurabhdaware
Copy link
Contributor Author

Need to do a tiny change. I'll mark it for review after that.

@abbottry
Copy link
Member

abbottry commented Oct 5, 2020

I think that the change necessary here is to just add e.stopPropagation() within handleClick for the CustomDataLinksToggle component. If that does the trick please ensure that the z-index for the rows look correct, there may be a change necessary.

@saurabhdaware
Copy link
Contributor Author

Yup right! e.stopPropagation() worked. Also, didn't need to change z-index
screenshot of collection page in table view that shows dropdown items

@saurabhdaware saurabhdaware marked this pull request as ready for review October 5, 2020 16:11
@saurabhdaware saurabhdaware changed the title EDSC-2895: Add isDownloadClicked condition to rowClick EDSC-2895: Fix download granule dropdown in table view Oct 5, 2020
@mreese84
Copy link
Contributor

mreese84 commented Oct 5, 2020

@saurabhdaware good deal. However, notice in your screenshot that there is a green line that goes over the top of the dropdown list. That should be under the dropdown.

Also, a reminder to make sure you have a test that covers this bugfix. Thanks!

@saurabhdaware
Copy link
Contributor Author

Yup, I'll make those changes and add tests. Thank you!

@saurabhdaware
Copy link
Contributor Author

I've used React.createPortal to render the menu outside of the table row. This does solve the z-axis issue.
image

I'll write tests now. Let me know if this is fine. Thanks!

Copy link
Collaborator

@trevorlang trevorlang left a comment

Choose a reason for hiding this comment

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

@saurabhdaware, this is looking awesome. I have one small suggestion. Once that's patched up, this should be good to go.

@@ -76,6 +77,9 @@ describe('GranuleResultsDataLinksButton component', () => {

describe('with multiple granule links', () => {
test('renders the correct element', () => {
// Mocks createPortal method of ReactDOM (https://stackoverflow.com/a/60953708/8116576)
ReactDOM.createPortal = jest.fn(modal => modal);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's strange to use modal in this case. I would prefer to see this named dropdown or something more specific to this implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right! I'll make that change.

Also, we will have to write a test that checks if dropdown opens when it is clicked from table view. (This test only checks if it opens when there are more links). I'll add that test as well then we can merge I guess.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is correct. And once this is done, it'll be ready to merge!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this and added a test for CustomDataLinksToggle component

@saurabhdaware
Copy link
Contributor Author

Hi, I've added test for CustomDataLinksToggle function that checks if e.stopPropagation and other required methods are fired on click. Let me know if you want any other changes. Thanks!

Copy link
Collaborator

@trevorlang trevorlang left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@trevorlang trevorlang merged commit 12ba091 into nasa:master Oct 9, 2020
@mreese84
Copy link
Contributor

mreese84 commented Oct 9, 2020

@saurabhdaware thank you for your contribution! This fix will go out in our 1.130.x release, which is scheduled for deployment to our Prod environment no later than 10/28/20. Thanks!

search.earthdata.nasa.gov

@saurabhdaware saurabhdaware deleted the fix-granule-download branch October 9, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EDSC-2895: Direct download button not working for multi-file granules in list view
5 participants