usnistgov / oar-pdr

The NIST Open Access to Research (OAR) Public Data Repository (PDR) system software
11 stars 10 forks source link

Data Cart Restructuring Pt. 2 #185

Closed RayPlante closed 3 years ago

RayPlante commented 3 years ago

As part of the on-going restructuring of the landing page code, PR #166 ("ODD-940: Data Cart code restructuring") was aimed at improving encapsulation of the code that manages the data cart. That PR centered on the introduction of the DataCart class that controls the list of items currently in the cart. This follow-on PR focuses on improving the encapsulation, particularly around the communication between components that use the cart as well as improving the separation of concerns for the different classes within the data cart module.

This PR attempts to address the these main concerns:

This PR addresses these concerns primarily through these design changes:

I have also added a description of the Cart framework into datacart/README.md that attempts to describe the role of the key classes and explain the communication between them.

chuanlin2018 commented 3 years ago

I found three issues and two of them were fixed:

  1. For ediid starts with "ark:", zip file name and download status do not show up in tree table (fixed);
  2. Download status in the tree table is always green (fixed);
  3. In the tree table, select/deselect the root of a dataset is not working properly. this.dataTree.children = [...this.dataTree.children] doesn't seem to be working. Function refreshTree() can be used as a work around but the tree table will fresh every time user select/deselect a treenode. So far I didn't find a better way to fix this.
RayPlante commented 3 years ago

This is great--thanks for doing this. The refreshTree() looks fairly inexpensive--nice! Hopefully it won't cause any weird blinking. I'll follow up with you on the next step.

chuanlin2018 commented 3 years ago

It does cause blinking :( So I didn't implement the change in this check in. You can try it to see if it's acceptable.

RayPlante commented 3 years ago

@chuanlin2018: This last commit (2f9e6d6) hopefully fixes the buggy selection behavior we were seeing (including # 3 in you list above). Can you confirm?

chuanlin2018 commented 3 years ago

Will do.

RayPlante commented 3 years ago

I'll note that this fix did not require the "who" capability that I shared in the restruct/cartservice-selwho branch, so I left that out.

chuanlin2018 commented 3 years ago

OK. I am reading your code while compiling locally.

chuanlin2018 commented 3 years ago

The problem seems to be fixed. I didn't see it happen again when running locally. Need a while to understand your code though:)

chuanlin2018 commented 3 years ago

Thanks for the explanation. Now I understand. Seems the conflicts need be resolved before I can approve the changes...

chuanlin2018 commented 3 years ago

Just ran into couple of minor issues while going through your code:

  1. "Remove Downloaded" and "Remove Selected" did the same thing - both buttons call the same function.
  2. Direct download in datacart was not working - problem with the parameters in this.dataCart.setDownloadStatus().
chuanlin2018 commented 3 years ago

I fixed the following:

  1. "Remove Downloaded" - now calls right function.
  2. Direct download in datacart
  3. gaTrackEvent() in Google Analytics service - handle the use case that event = undefined while action = undefined. So far this never happened.