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

feat: display metadata on project landing page (DSP-1065) #348

Merged
merged 9 commits into from Jan 11, 2021

Conversation

@waychal
Copy link
Contributor

@waychal waychal commented Dec 17, 2020

resolves DSP-1065

This is the first draft of the project landing page.

In this PR, most of the metadata is hard-coded to finish the functionality. As the test data is very limited, it is difficult to handle different cases. In next iterations, hard-coded values will be removed.

This is my first significant PR in DSP-APP and also using Angular 9. So looking forward for reviews and improvements. :)

@waychal waychal requested review from kilchenmann and mpro7 Dec 17, 2020
@waychal waychal self-assigned this Dec 17, 2020
<p style="margin-top: 4px;">
URL: <a href="{{ metadata['dataManagementPlan']['url']['value'] }}" target="_blank"> {{ metadata['dataManagementPlan']['url']['value'] }} </a>
</p>
<p>isAvailable: {{ metadata['dataManagementPlan']['isAvailable'] }}</p>
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

Instead of displaying the boolean value, basing on it I would display:
metadata.dataManagementPlan.isAvailable ? 'Available' : 'Unavailable'
But then the url shouldn't be also displayed or exist. These are just the dummy data, it needs to be clarified.

Loading

Copy link
Contributor Author

@waychal waychal Dec 18, 2020

Choose a reason for hiding this comment

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

Done. To make changes, I have updated the hard-coded metadata for metadata.dataManagementPlan.isAvailable = true.

Loading

<!-- #################### template for dataManagementPlan ################# -->
<div *ngSwitchCase="'spatialCoverage'">
<div *ngFor="let coverage of metadata['spatialCoverage'] | keyvalue;">
<a href="{{ coverage.value['place']['url'] }}" target="_blank"> {{ coverage.value['place']['name'] }} </a>
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

As a link title for now I would display the link itself {{ coverage.value['place'] }}. But later on we need to think about better solution (like parsing for the location name).

Loading

Copy link
Contributor Author

@waychal waychal Dec 18, 2020

Choose a reason for hiding this comment

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

Instead of displaying object, for now I am displaying it as:
coverage.value['place']['name'] ( coverage.value['place']['url'] )

For example:
Geonames ( https://www.geonames.org/2017370/russian-federation.html )

Loading

@@ -0,0 +1,28 @@
<h1>Selected dataset: {{ metadata['title'] }} </h1>

<p class="note">Click on the radio button in side panel to change the dataset.</p>
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

This information I would display only if there is more than one Dataset available. Also I would change this message to: "Default dataset is displayed, to change it... "

Loading

Copy link
Contributor Author

@waychal waychal Dec 18, 2020

Choose a reason for hiding this comment

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

Done.

Loading

@@ -0,0 +1,15 @@
<h1>Attribution</h1>

<p>The following people are involved in making of dataset.</p>
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

I would change it to The following people are involved in creation of the dataset:.

Loading

Copy link
Contributor Author

@waychal waychal Dec 18, 2020

Choose a reason for hiding this comment

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

Done.

Loading

Copy link
Collaborator

@flavens flavens Jan 4, 2021

Choose a reason for hiding this comment

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

to correct: "in THE creation of the dataset"

Loading

<h1>Contact</h1>

<div class="tab-contents">
<p style="margin-bottom: 40px;">If you have any questions, please contact us on </p>
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

I would remove it.

Loading

Copy link
Contributor Author

@waychal waychal Dec 18, 2020

Choose a reason for hiding this comment

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

Done.

Loading

<div class="content large middle">
<!-- mobile version: status and edit icon displayed before the title -->
<div class="app-toolbar-mobile">
<span class="app-toolbar-action-status">
<span class="badge status" [class.active]="project.status">
<span *ngIf="project.status">Active</span>
<span *ngIf="!project.status">Deactivated</span>
</span>
</span>
<span class="fill-remaining-space-action"></span>
<span class="app-toolbar-action-edit">
<span class="fill-remaining-space-action"></span>
<span class="app-toolbar-action-edit">
<button mat-icon-button class="right"
(click)="openDialog('editProject', project.shortname, project.shortcode)"
*ngIf="projectAdmin && project.status">
(click)="openDialog('editProject', project.shortname, project.shortcode)"
*ngIf="projectAdmin && project.status">
<mat-icon>edit</mat-icon>
</button>
</span>
</div>
</div>

<!-- desktop and tablet version -->
<div class="app-toolbar transparent more-space-bottom">
<div class="app-toolbar-row">
<h3 class="mat-body subtitle">
Project {{project.shortcode}} | {{project.shortname | uppercase}}
</h3>
<span class="fill-remaining-space"></span>
<span class="app-toolbar-action">
<!-- desktop and tablet version -->
<div class="app-toolbar transparent more-space-bottom">
<div class="app-toolbar-row">
<h3 class="mat-body subtitle">
Project {{ project.shortcode }} | {{ project.shortname | uppercase }}
</h3>
<span class="fill-remaining-space"></span>
<span class="app-toolbar-action">
<span class="badge status" [class.active]="project.status">
<span *ngIf="project.status">Active</span>
<span *ngIf="!project.status">Deactivated</span>
</span>
</span>
</div>
<div class="app-toolbar-row">
<h2 class="mat-title">
{{project.longname}}
</h2>
<span class="fill-remaining-space"></span>
<span class="app-toolbar-action">
<button mat-icon-button class="right"
(click)="openDialog('editProject', project.shortname, project.shortcode)"
*ngIf="projectAdmin && project.status">
</div>
<div class="app-toolbar-row">
<h2 class="mat-title">
{{ project.longname }}
</h2>
<span class="fill-remaining-space"></span>
<span class="app-toolbar-action">
<button mat-icon-button
*ngIf="projectAdmin && project.status"
(click)="openDialog('editProject', project.shortname, project.shortcode)"
class="right">
<mat-icon>edit</mat-icon>
</button>
</span>
</div>
</div>
</div>
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

It is something wrong with formatting here. Inner elements are wrongly indented.

Loading

Copy link
Contributor Author

@waychal waychal Dec 18, 2020

Choose a reason for hiding this comment

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

Thanks for noticing it. I normally use Webstorm auto-indentation but sometimes it doesn't work properly for <span> element. I have corrected it now.

Loading



.more-space-bottom {
margin-bottom: 15px !important;
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

If possible avoid using !important.

Loading

Copy link
Contributor Author

@waychal waychal Dec 18, 2020

Choose a reason for hiding this comment

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

It is old code and I am not sure how it will affect if I remove !important.
@kilchenmann can you tell me if it is safe to remove. It is for desktop and tablet version. If I remove it, I don't see any difference.

Loading

}

// grid
::ng-deep .properties-container {
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

::ng-deep is deprecated, however there is still no replacement for this. So we need to keep it in mind to not use it if not necessary.

Loading

Copy link
Contributor Author

@waychal waychal Dec 18, 2020

Choose a reason for hiding this comment

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

properties-container class is used in many children components. So instead of repeating style I have defined the style in parent component with ::ng-deep. If there is better solution, I am happy to change it.

Loading

Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

As above, I don't mind to use it, but once they deprecate it for good (it is still marked "to deprecate" in Angular 9), someone needs to fix it.

Loading

metadata: object;
datasetMetadata: object;
projectMetadata: object;
attribution: object;
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

Primitive object types could be replaced by interfaces, but it can be also done in another iteration since the data model is kind of liquid ;)

Loading

@@ -100,13 +112,174 @@ export class BoardComponent implements OnInit {
this.loading = false;
}

getProjectMetadata() {
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

This could be already replaced by DSP-JS method call with http, but the payload need to be uploaded first, since there is no default data in the database. Maybe you should request backend to add it.
But because this is still WIP, it is fine to have all hardcoded.

Loading

Copy link
Contributor Author

@waychal waychal Dec 18, 2020

Choose a reason for hiding this comment

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

I will add Ajax call to get metadata in next iteration.

Loading

Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

HttpClient from Angular common module.

Loading

@waychal waychal requested a review from mpro7 Dec 18, 2020
Copy link
Contributor

@mpro7 mpro7 left a comment

I will leave it open for now. Someone else from APP developers should also have a look and finally approve it. Either @flavens or @kilchenmann

Please write down all things which are addressed for next iteration and add them to its task description, to don't lose it.

Loading

package.json Outdated
@@ -47,6 +47,7 @@
"json2typescript": "^1.0.6",
"jsonld": "^1.1.0",
"moment": "^2.27.0",
"ngx-clipboard": "^14.0.1",
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

Ok, check this out in next iteration.

Loading

<div class="sub-details">
<h4>Address:</h4>
<address class="contents">
<p *ngIf="address['streetAddress']">{{ address['streetAddress'] }}</p>
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

Is this happening everywhere? This is strange, because you use hardcoded data. However you can also use ? operator to check if key is undefined, then it shouldn't return error if it is:

address.streetAddress?.anotherProp?

But if you use primitive object type and get this error: Identifier 'test' is not defined. 'object' does not contain such a member then it is clear as error says. So you can leave it now, but in second iteration when you add interfaces, bracket notation should be easily replaced with dot notation.

Loading


ngOnInit(): void {
// reformat data
for (let idx = 0; idx < this.people.length; idx++) {
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

But why do you think this could be reused anywhere?

Loading

}

// grid
::ng-deep .properties-container {
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

As above, I don't mind to use it, but once they deprecate it for good (it is still marked "to deprecate" in Angular 9), someone needs to fix it.

Loading

@@ -100,13 +112,174 @@ export class BoardComponent implements OnInit {
this.loading = false;
}

getProjectMetadata() {
Copy link
Contributor

@mpro7 mpro7 Dec 18, 2020

Choose a reason for hiding this comment

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

HttpClient from Angular common module.

Loading

@mpro7 mpro7 requested a review from flavens Dec 18, 2020
@waychal
Copy link
Contributor Author

@waychal waychal commented Dec 22, 2020

@mpro7 I have updated task in Youtrack for the todos in next iteration.

Loading

Copy link
Collaborator

@flavens flavens left a comment

looks good! but there are changes to make!
question: is it possible to use the value components to display the information? (Text as string, url, date)

Loading

src/app/project/board/board.component.html Show resolved Hide resolved
Loading
src/app/project/board/board.component.html Show resolved Hide resolved
Loading
<div *ngIf="excludeKeys.indexOf(prop.key) < 0" class="property">
<div class="property-label">
<h3 class="label mat-subheading-1">
{{ prop.key }}
Copy link
Collaborator

@flavens flavens Jan 4, 2021

Choose a reason for hiding this comment

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

is there a way to get the label instead of the key? because the keys are not well written (e.g. startDate, dataManagementPlan)
same question for the panel "Dataset"

Loading

Copy link
Contributor Author

@waychal waychal Jan 4, 2021

Choose a reason for hiding this comment

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

I agree but it requires changes in the backend. Currently I am using the format received from Js-lib. If we want to use label, we need to update this response format.
I would suggest to update the response format (in backend) in next iteration. So that I can directly receive it in frontend using metadata endpoint and use it instead of hardcoding it now.

Loading

Copy link
Collaborator

@flavens flavens Jan 4, 2021

Choose a reason for hiding this comment

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

ok,I think it is important to have it soon

Loading

Copy link
Contributor

@mpro7 mpro7 Jan 5, 2021

Choose a reason for hiding this comment

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

I'm not API designer specialist, but I think common practice is to use lowerCamelCase for properties to avoid spaces and other strange characters. I've never also worked with in this way designed API, however it's possible to find this kind of ideas in the web, I'm not sure if this is best practice. @tobiasschweizer what do you think about it?

Loading

Copy link
Contributor

@tobiasschweizer tobiasschweizer Jan 5, 2021

Choose a reason for hiding this comment

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

Sorry, I think I am missing some context here. Could you give me some examples?

Loading

Copy link
Contributor

@mpro7 mpro7 Jan 5, 2021

Choose a reason for hiding this comment

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

I think I talked about it with you already. Snehal would like to have Space Separated properties instead of lowerCamelCase, as we have now, just to iterate through and display it directly on the view. But I've just understood it is rather not doable.

Loading

Copy link
Contributor

@tobiasschweizer tobiasschweizer Jan 5, 2021

Choose a reason for hiding this comment

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

You can surely have display strings (labels) that contain spaces, but property names cannot have spaces. So I think you are right about the camel case convention.

Loading

Copy link
Contributor Author

@waychal waychal Jan 7, 2021

Choose a reason for hiding this comment

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

Let's discuss this in next metadata discussion meeting. I am adding @BalduinLandolt for reference here as he is coordinating the metadata group.

Loading

Loading
Loading
'givenName': 'Stewart',
'jobTitle': 'Dr.',
'memberOf': 'http://ns.dasch.swiss/test-dasch',
'sameAs': {'type': 'https://schema.org/URL', 'value': 'https://orcid.org/0000-0002-1825-0097'},
Copy link
Collaborator

@flavens flavens Jan 4, 2021

Choose a reason for hiding this comment

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

what does sameAs mean? Is it possible to have a more explicit label for this field?

Loading

Copy link
Contributor Author

@waychal waychal Jan 7, 2021

Choose a reason for hiding this comment

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

From my understanding, sameAs contains the profile url. It is the key I received in REST API response. I will add this point in metadata discussion if we want to change it. I am adding @BalduinLandolt and @mpro7 for reference.

Loading

Copy link
Collaborator

@flavens flavens Jan 8, 2021

Choose a reason for hiding this comment

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

I guess you can find a better label. I really doubt that a lambda user will understand what sameAs means. You can also discuss it with Marion or Rita.

Loading

src/app/project/board/board.component.html Show resolved Hide resolved
Loading
Loading
@waychal
Copy link
Contributor Author

@waychal waychal commented Jan 7, 2021

question: is it possible to use the value components to display the information? (Text as string, url, date)

What do you mean by value component here? Can you give more details?

Loading

@waychal
Copy link
Contributor Author

@waychal waychal commented Jan 7, 2021

@flavens I have also merged main branch into this branch. There was big conflict in package-lock.json file. Can you please verify it? Thanks.

Loading

@waychal waychal requested a review from flavens Jan 7, 2021
@flavens
Copy link
Collaborator

@flavens flavens commented Jan 8, 2021

question: is it possible to use the value components to display the information? (Text as string, url, date)

What do you mean by value component here? Can you give more details?

in dsp-ui > viewer module, we defined several component to handle the creation/edition/deletion of property values. We use them in the resource viewer. Your case seems similar even though the project properties here are different.

Read the design doc for more info about it.

Screenshot 2021-01-08 at 09 26 33

Loading

@flavens
Copy link
Collaborator

@flavens flavens commented Jan 8, 2021

@flavens I have also merged main branch into this branch. There was big conflict in package-lock.json file. Can you please verify it? Thanks.

It is still relevant? I cannot see any conflicts.

Loading

@mpro7 mpro7 changed the title DSP-1065 Display metadata on project landing page feat: display metadata on project landing page (DSP-1065) Jan 8, 2021
@mpro7
Copy link
Contributor

@mpro7 mpro7 commented Jan 8, 2021

@flavens I have also merged main branch into this branch. There was big conflict in package-lock.json file. Can you please verify it? Thanks.

After merging main to the branch you should run also npm install to avoid this situation. This file actually contains some important logs, but importance actually depends how and if those information are used. Here you can read how to deal with this situation.

Loading

@mpro7 mpro7 self-requested a review Jan 11, 2021
mpro7
mpro7 approved these changes Jan 11, 2021
Copy link
Contributor

@mpro7 mpro7 left a comment

Please create a list with non-addressed questions/issues and add those to next step task. It would be better however to split the big task to small ones, then PR will be not that complex and easier to handle.

Loading

@waychal waychal merged commit 3012ef5 into main Jan 11, 2021
4 checks passed
Loading
@waychal waychal deleted the DSP-1065-add-metadata-on-project-landing-page branch Jan 11, 2021
@waychal
Copy link
Contributor Author

@waychal waychal commented Jan 11, 2021

question: is it possible to use the value components to display the information? (Text as string, url, date)

What do you mean by value component here? Can you give more details?

in dsp-ui > viewer module, we defined several component to handle the creation/edition/deletion of property values. We use them in the resource viewer. Your case seems similar even though the project properties here are different.

Read the design doc for more info about it.

Screenshot 2021-01-08 at 09 26 33

I was planning to use this value component at first but as I have only read only properties, Andre asked me to not use it and create the list separately.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants