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(lists): read mode project member (DEV-1343) #825

Merged
merged 13 commits into from Sep 20, 2022

Conversation

Vijeinath
Copy link
Collaborator

resolves DEV-1343
The lists (within the list tab and on the sidenav) are visible for project member but all the CRUD operations regarding lists were hidden.

@mdelez mdelez changed the title Wip/dev 1343 read mode project member feat(lists): read mode project member (DEV-1343) Sep 16, 2022
Copy link
Collaborator

@mdelez mdelez left a comment

Choose a reason for hiding this comment

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

In addition to the other comments, the indentation needs to be worked on a bit. I think it's tricky because you have to account for adding extra space if the expand/collapse button is hidden.

Screen Shot 2022-09-16 at 15 20 48

Screen Shot 2022-09-16 at 15 21 27

@@ -2,28 +2,28 @@
<div class="list-node" *ngFor="let node of list; let first = first; let last = last;">

<!-- button to expand / close node -->
<button type="button" mat-icon-button (click)="toggleChildren(node.id)" class="">
<button *ngIf="node.children.length !== 0" type="button" mat-icon-button (click)="toggleChildren(node.id)">
Copy link
Collaborator

Choose a reason for hiding this comment

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

This causes the arrow to be hidden even for project admins. The arrow needs to be shown for project admins so that they can add child node even if one doesn't already exist.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in cd87150

<mat-icon class="mat-icon-rtl-mirror">
{{showChildren(node.id) ? 'expand_more' : 'chevron_right'}}
</mat-icon>
</button>

<div class="list-item-indentation" *ngIf="node.children.length === 0"></div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of doing this, I would add a conditional class on the button above in the event that the node has no children. Something like [class.list-item-indentation]="node.children.length === 0".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried with the class condition but couldn't do it. So I changed the whole situation by adding a div around the button with a certain size -> cd87150

Copy link
Collaborator

@mdelez mdelez left a comment

Choose a reason for hiding this comment

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

just a small thing and then I will approve :)

projectAdmin = false;

// project data
project: ReadProject;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this variable is needed, you can use a const to temporarily store the project you get from the cache

Copy link
Collaborator Author

@Vijeinath Vijeinath Sep 20, 2022

Choose a reason for hiding this comment

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

You are right, it is not needed elsewhere (ff8b5a8)

// get the project data from cache
this._cache.get(this.projectCode).subscribe(
(response: ReadProject) => {
this.project = response;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use a const here instead

Copy link
Collaborator Author

@Vijeinath Vijeinath Sep 20, 2022

Choose a reason for hiding this comment

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

I would go further and not even use const but pass the response directly because it is not needed elsewhere (ff8b5a8)

projectAdmin = false;

// project data
project: ReadProject;
Copy link
Collaborator

Choose a reason for hiding this comment

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

same thing here as in the list-item-form component

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -26,8 +26,10 @@ import { OntologyService } from './ontology/ontology.service';
styleUrls: ['./project.component.scss']
})
export class ProjectComponent implements OnInit {
readonly TAB_DATA_MODEL = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks

Copy link
Collaborator

@mdelez mdelez left a comment

Choose a reason for hiding this comment

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

works well, thanks!

@Vijeinath Vijeinath merged commit b818288 into main Sep 20, 2022
@Vijeinath Vijeinath deleted the wip/dev-1343-read-mode-project-member branch September 20, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants