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

DSP-369 System Prop Info #157

Merged
merged 16 commits into from
Sep 23, 2020
Merged

DSP-369 System Prop Info #157

merged 16 commits into from
Sep 23, 2020

Conversation

mdelez
Copy link
Contributor

@mdelez mdelez commented Aug 20, 2020

@mdelez mdelez added the enhancement New feature or request label Aug 20, 2020
@mdelez mdelez self-assigned this Aug 20, 2020
@@ -123,6 +123,10 @@ export class DisplayEditComponent implements OnInit {
this.readOnlyValue = this._valueTypeService.isReadOnly(this.valueTypeOrClass);
}

getTooltipText(): string {
return 'Value creation date: ' + this.displayValue.valueCreationDate + '\n Attached to user: ' + this.displayValue.attachedToUser;
Copy link
Contributor

Choose a reason for hiding this comment

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

does a newline take any effect in html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it does

Copy link
Contributor

Choose a reason for hiding this comment

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

but isn't it treated like a newline (rather than an actual line break)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah, it doesn't generate a <br>. Here's the generated HTML:
Screen Shot 2020-09-17 at 11 03 57

@mdelez mdelez requested a review from flavens September 21, 2020 14:06

getTooltipText(): string {
return 'Value creation date: ' + this.displayValue.valueCreationDate +
'\n Attached to user: ' + this.user?.givenName + ' ' + this.user?.familyName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would keep the labels simple: "Creation date" + "Value owner" or "Value creator".
According to the knora docs, attachedToUser = The user who owns the value. You can pick the most appropriate term in English.

Copy link
Contributor

@kilchenmann kilchenmann Sep 22, 2020

Choose a reason for hiding this comment

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

I agree with "Creation date" and "Value creator" 👍
"Value owner" can confuse users especially when the value is created by an import script. In this case the owner is a bot or an import user. This will drive people crazy 🤪

Copy link
Contributor Author

@mdelez mdelez Sep 22, 2020

Choose a reason for hiding this comment

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

fixed in 77aaefd. Apparently the new line character never actually worked. It was just a coincidence that the line broke at the correct place. I needed to add white-space: pre-line to the tooltip class in dsp-ui.scss.

@@ -31,6 +31,12 @@
(click)="activateEditMode()">
<mat-icon>edit</mat-icon>
</button>
<button mat-button
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering if this information should not also be readable in read mode..
@kilchenmann what do you think? how was it in Salsah?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand: What information is meant here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

here there are Creation date and Value creator. Do we need to display other kind of information?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see...it's the creation date and value creator etc

Copy link
Contributor

Choose a reason for hiding this comment

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

This information does not exist in SALSAH; Neither on the resource nor on the propert.
Here's a resource view:
https://data.dasch.swiss/resources/2632709

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about also having the value comment button show on hover? I just realized there is no way to see the comment in read mode currently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yes I think it is important to have it in read mode, and if I've understood correctly, it is possible that a user can have the permission to create/edit a value comment but does not have the permission to create/edit/delete a value.... but this is another issue. @kilchenmann what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is possible that a user can have the permission to create/edit a value comment but does not have the permission to create/edit/delete a value.... but this is another issue.

Yes this should be solved in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4d93d5e enables the property info and comment hover buttons in read mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is possible that a user can have the permission to create/edit a value comment but does not have the permission to create/edit/delete a value.... but this is another issue.

Yes this should be solved in a separate PR.

@flavens I've created a user story for this and added it to the dsp-ui roadmap. Not sure when we'll get around to it.
https://dasch.myjetbrains.com/youtrack/issue/DSP-683

@mdelez mdelez requested a review from flavens September 22, 2020 12:53
Copy link
Collaborator

@flavens flavens left a comment

Choose a reason for hiding this comment

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

LGTM

@mdelez mdelez merged commit 54cee2e into master Sep 23, 2020
@mdelez mdelez deleted the wip/dsp-369-system-prop-info branch September 23, 2020 12:49
@kilchenmann kilchenmann changed the title System Prop Info DSP-369 System Prop Info Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants