New science themes - Githubissues

usnistgov / oar-pdr

The NIST Open Access to Research (OAR) Public Data Repository (PDR) system software

11 stars 10 forks source link

New science themes #242

Closed chuanlin2018 closed 2 years ago

chuanlin2018 commented 2 years ago

This is the latest version of science theme landing page. UI screenshot attached. Did makedist, testall and server side rendering test locally. Going to test it in local docker.

To test it locally, set useMetadataService: true, useCustomizationService: true in environment.ts and set oardev as backend service.

Screen Shot 2022-04-21 at 5 32 57 PM

chuanlin2018 commented 2 years ago

Thanks for the detail comments. Just read it through. We can discuss it during or after the standup meeting.

From: Ray Plante @.> Date: Thursday, May 12, 2022 at 8:34 AM To: usnistgov/oar-pdr @.> Cc: Lin, Chuan (Assoc) @.>, Author @.> Subject: Re: [usnistgov/oar-pdr] New science themes (PR #242)

@RayPlante requested changes on this pull request.

See ODD OneNote notes for changes discussed in our group review.

I've made a number of in-line comments of things that should be addressed. We should get together to discuss which of these we should do before merging this PR and which we handle in a follow-up task.

In angular/src/app/landing/version/version.component.htmlhttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871169886:

 <span class="" *ngIf="record.issued" style="padding-right: 10pt;"> Released:

     <strong>{{record.issued.slice(0,10)}}</strong></span>

 <span class="" *ngIf="record.modified" style="padding-right: 10pt;"> Last modified:

     <strong>{{record.modified.slice(0,10)}}</strong></span>

<span class="" *ngIf="record.creators" style="padding-right: 10pt;"> Created by:

This VersionCompnent should only include information related to the version and version history. Please move this out to the AboutdatasetComponent with facilitators.

In angular/src/app/frame/headbar.component.tshttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871175851:

@@ -43,6 +43,7 @@ export class HeadbarComponent {

 public EDIT_MODES: any;

 public CART_CONSTANTS: any = CartConstants.cartConst;

 globalCartUrl: string = "/datacart/" + this.CART_CONSTANTS.GLOBAL_CART_NAME;

theme: string = "nist";

It doesn't look like theme is being used in this class.

In angular/src/app/landing/filters/filters.component.htmlhttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871191937:

data-target="#demo"><i [ngClass]="getFilterImgClass()"

style="zoom: 100%;color: #fff;cursor: pointer;">
<a href="#" id="clear-all"
(click)="$event.preventDefault();clearFilters()">

Clear All

Seeing the word, "forensics", in this template suggests that this is not very general. Suppose we come up with another vocabulary to support; will this template need to be updated? You don't need to fix this now, but please think about how we can make this more general by reacting to vocabularies that are in the NERDm metadata.

In angular/src/app/landing/landingpage.component.tshttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871198050:

@@ -165,6 +171,7 @@ export class LandingPageComponent implements OnInit, AfterViewInit {

     let metadataError = "";

     this.displaySpecialMessage = false;

     this.CART_ACTIONS = CartActions.cartActions;

this.imageURL = 'assets/images/fingerprint.jpg';

We'll make this more general in a later release.

In angular/src/app/landing/sections/resourcedata.component.htmlhttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871202777:

     <!-- Display Homepage button if 'landingPage field exist and does not contain '/od/id' -->

<div *ngIf="record['landingPage'] && record['landingPage'].indexOf('/od/id') === -1 ">
<div *ngIf="theme != scienceTheme && record['landingPage'] && record['landingPage'].indexOf('/od/id') === -1 ">

I think that we should allow a science theme to have an external landing page. Can we remove this theme conditional?

In angular/src/app/landing/sections/resourcedata.component.tshttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871284872:

@@ -74,8 +84,14 @@ export class ResourceDataComponent implements OnChanges {

  *                  full list from the NERDm Resource record

  */

 selectAccessPages(comps : NerdmComp[]) : NerdmComp[] {

let use : NerdmComp[] = comps.filter(cmp => @.***'].includes("nrdp:AccessPage") &&
! @.***'].includes("nrd:Hidden"));
let use : NerdmComp[];

I'd like to understand this logic a bit more. It looks it assumes that a ScienceTheme record cannot have AccessPage components, which is not true. But let's talk about it.

In angular/src/app/landing/sections/resourceidentity.component.htmlhttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871288863:

 </div>

<span *ngIf="showHomePageLink" style="float: right;">
<span *ngIf="showHomePageLink && recordType!='Science Theme'" style="float: right;">

Allow external home pages? See comment above for resourcedata.component.

In angular/src/app/nerdm/nerdm.tshttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871299637:

public theme(): string {

let theme : string = Themes.DEFAULT_THEME;
if @.'] instanceof Array && @.'].length > 0) {
return @.***'][0].replace(/\s/g, '').split(":")[1]);
}
return theme;
}
/**
- Return science theme search url
*/
public scienceThemeSearchUrl() {
if(this.theme() == Themes.SCIENCE_THEME) {
return this.data['components'][0].searchURL;

This seems to make several assumptions about what components are present in a Science Theme record and in what order. (I'm also now unclear about the meaning of "theme" here is.) Let's talk about making this more general.

In angular/src/app/shared/globals/globals.tshttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871302190:

@@ -0,0 +1,41 @@

+export class Themes {

static readonly SCIENCE_THEME = 'ScienceTheme';
static readonly DEFAULT_THEME = 'DefaultTheme';

Some documentation in this class would be helpful to understand what a "theme" is.

In angular/src/app/shared/search-service/search-service.service.tshttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871307792:

@@ -117,6 +124,48 @@ export class SearchService {

/**
- Returns an Observable for the HTTP GET request for the JSON resource.
- @return {string[]} The Observable for the HTTP request.
*/
searchPhrase(url: string): Observable {

This function does not do what the name suggests, and the documentation does not seem correct either. Please rename this function to properly reflect what it actually does. Also, please update the documentation that explains what is implied by the name; if there are meant to be any assumptions about what the input URL is meant to return (which I think there are), please explain them.

In angular/src/app/shared/search-service/search-service.service.tshttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r871312336:

@@ -117,6 +124,48 @@ export class SearchService {

/**
- Returns an Observable for the HTTP GET request for the JSON resource.
- @return {string[]} The Observable for the HTTP request.
*/
searchPhrase(url: string): Observable {
console.log('search url', url);
return this.http.get(this.portalBase+url);
}
/**
- Watch current page
*/
watchCurrentPage(): Observable {

I find the last four functions in this class starting with this one problematic in several ways. For one, it's not clear they have anything to with searching and thus should probably not be part of this class.

Fortunately, these functions do not appear to be used anywhere in the application. Can we just remove them?

— Reply to this email directly, view it on GitHubhttps://github.com/usnistgov/oar-pdr/pull/242#pullrequestreview-970554456, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKQHD3GM24VIB2ZIXIHZBE3VJT3FXANCNFSM5UAPQTBA. You are receiving this because you authored the thread.Message ID: @.***>

chuanlin2018 commented 2 years ago

@RayPlante, I believe I have fixed all the bugs for this release. Please review again. Thanks. -Chuan

RayPlante commented 2 years ago

I believe that is correct. The idea is that it will still display any AccessPage/SearchPage components that are not also DynamicResourceSet components.

From: chuanlin2018 @.> Sent: Tuesday, May 17, 2022 2:29 PM To: usnistgov/oar-pdr @.> Cc: Plante, Raymond L. (Fed) @.>; Mention @.> Subject: Re: [usnistgov/oar-pdr] New science themes (PR #242)

@chuanlin2018 commented on this pull request.

In angular/src/app/landing/sections/resourcedata.component.tshttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r875143117:

@@ -74,8 +88,10 @@ export class ResourceDataComponent implements OnChanges {

  *                  full list from the NERDm Resource record

  */

 selectAccessPages(comps : NerdmComp[]) : NerdmComp[] {

let use : NerdmComp[] = comps.filter(cmp => @.***'].includes("nrdp:AccessPage") &&
! @.***'].includes("nrd:Hidden"));
let use : NerdmComp[];
use = comps.filter(cmp => @.***'].includes("nrdp:AccessPage") &&
! @.***'].includes("nrd:Hidden"));

Okay. No change to html part is necessary then, correct?

- Reply to this email directly, view it on GitHubhttps://github.com/usnistgov/oar-pdr/pull/242#discussion_r875143117, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAH245GB4D6HQVB36H3AQ2TVKPQO7ANCNFSM5UAPQTBA. You are receiving this because you were mentioned.Message ID: @.**@.>>

chuanlin2018 commented 2 years ago

After I checked in the code, I realized that there was still a logic problem here:

selectAccessPages() will only be called if it's "default theme". So the new code added will never be executed... I think I am still not quite clear what to display in default theme vs science theme.

Screen Shot 2022-05-17 at 4 37 32 PM

RayPlante commented 2 years ago

I have a recommendation. It is based first on the fact that, from the schema definition perspective, any record is allowed to have AccessPage, SearchPage, and DynamicResourceSet components, so the default theme should attempt to display them all. Second, the "Science Theme" theme makes some assumptions about what the user would prefer to see emphasized.

Given that, I would change the above code to the following:

this.accessPages = [];
this.dynamicResourcePages = [];
if (this.record['components']) {
    this.dynamicResourcePages = this.selectDynamicResources(this.record['components']);
    this.accessPages = this.selectAccessPages(this.record['components']);
    if (this.theme == this.scienceTheme)
        this.accessPages = this.accessPages.filter(cmp => ! cmp['@type'].includes("nrda:DynamicResourceSet"));
}

Does this make sense?

RayPlante commented 2 years ago

At some point, could you merge integration into this branch and resolve the conflicts?

chuanlin2018 commented 2 years ago

Yes this make sense :) I will do the merge and then check in the new code.

chuanlin2018 commented 2 years ago

In above checkin, I merged integration into this branch and fixed access page logic.