whole-tale / ngx-dashboard

WholeTale Dashboard rewritten in Angular
MIT License
0 stars 3 forks source link

Create Tale from DOI #281

Closed craig-willis closed 2 years ago

craig-willis commented 2 years ago

Problem

Approach

How to Test

  1. Checkout this branch locally, rebuild the dashboard
  2. Login to the WholeTale Dashboard
  3. Create New Tale from DOI
    1. Enter DOI doi:10.7910/DVN/TJCLKP: Title should populate with the DVN dataset title
    2. Enter URL https://sandbox.zenodo.org/record/1059441: Modal should convert to import tale
    3. Enter a non-existent DOI, confirm that Failed to find DOI/URL message displays
    4. Confirm that radio buttons work as expected
  4. Create tale using AinWT
    1. https://dashboard.local.wholetale.org/mine?uri=doi%3A10.7910%2FDVN%2FTJCLKP&asTale=true
    2. https://dashboard.local.wholetale.org/mine?uri=doi%3A10.7910%2FDVN%2FTJCLKP&asTale=false
    3. https://dashboard.local.wholetale.org/mine?uri=doi%3A10.7910%2FDVN%2FTJCLKP (defaults to asTale=false)
    4. https://dashboard.local.wholetale.org/mine?uri=https%3A%2F%2Fsandbox.zenodo.org%2Frecord%2F1059441
  5. Optionally, confirm that original create tale and Create from Git work.

Known issues:

bodom0015 commented 2 years ago

Added a small fix for the radio buttons. Due to the nature of *ngIf, the templated section will only be added to the DOM if the condition is true, which can introduce a race condition. Therefore, if the condition is not true when Angular goes to init the view (e.g. due to HTTP requests or other async operations), Angular does not render it to the DOM. Because it's not on the DOM when Angular runs its ngOnInit and ngAfterView hooks, jQuery will run as a no-op when attempting to initialize these conditional components. This is true for any jQuery-based UI stuff we use (typically just Semantic UI).

One way to resolve the race condition is frequently call jQuery to re-init the element just in case it hasn't been initialized, but it can be tedious to track down all of the different places in the code where that condition can change.

The other way to resolve this is to use [hidden], which is effectively the opposite of *ngIf - that is, hidden can be used to hide things from the DOM, where *ngIf prevents them from being added at all. Note that hidden elements can still be initialized by jQuery properly, and that the condition logic is effectively opposite (e.g. "when should I add this to the DOM" vs "when should this be hidden from the view")

Also included a couple of other very minor styling fixes that tslint was complaining about:

craig-willis commented 2 years ago

Several things going on in this last commit:

Xarthisius commented 2 years ago

LGTM! The only nitpick I have is that I ran the test case without closing the modal. Specifically in point 3. I went from 3.ii to 3.iii and got a confusing result:

modal

I think it would be a nicer UX if in a case of failed DOI/URL the state of the modal would be reset.

craig-willis commented 2 years ago

Good catch. There may be a better way, but I've added a simple reset() function to reset modal fields on lookup error.

bodom0015 commented 2 years ago

This looks great! The reset logic seems to work well. One possible improvement might be to send the DOI search request automatically (after a debounce time, instead of requiring blur or pressing Enter), but that is outside the scope of this PR and could come with its own set of headaches that we should discuss first.

With your permission, I can also push a fix for these new merge conflicts and the minor build warnings here :+1:

craig-willis commented 2 years ago

With your permission, I can also push a fix for these new merge conflicts and the minor build warnings here

Permission granted!