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

Link value component #27

Merged
merged 40 commits into from
Mar 25, 2020
Merged

Link value component #27

merged 40 commits into from
Mar 25, 2020

Conversation

SepidehAlassi
Copy link
Contributor

@SepidehAlassi SepidehAlassi commented Feb 17, 2020

to do:
display:

  • show the target resource label in a read-only drop-down menu

edit:

  • search for all possible resources with given three letters of the label

  • list the resource labels of all possible target resources in a drop-down menu

  • read the user's choice and assign to the target of the link value

test

  • write tests for edit and display

closes #14

@SepidehAlassi SepidehAlassi self-assigned this Feb 17, 2020
@SepidehAlassi SepidehAlassi changed the title Wip/14 link value component Link value component Feb 18, 2020
@tobiasschweizer
Copy link
Contributor

Thanks for this PR, I will try to review it tomorrow.

@tobiasschweizer
Copy link
Contributor

please see my proposal #47

@tobiasschweizer
Copy link
Contributor

Shall I take another look?

@SepidehAlassi
Copy link
Contributor Author

Shall I take another look?

if you wish so

valueChangesSubscription: Subscription;
labelChangesSubscription: Subscription;
// label cannot contain logical operations of lucene index
customValidators = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a custom validator function is needed because all modes but update cannot check if the value is actually an instance of ReadResource. I guess you could define something like the following and just include it in the customValidators:

export function resourceValidator(control: AbstractControl) {
    const invalid = !(control.value instanceof ReadResource);
    return invalid ? {'invalidType': {value: control.value}} : null;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

the use case is here is more complex because the value of the FormControl is a string / label as long as the user types and an instance of ReadResource once the user selects from the list.

Copy link
Contributor Author

@SepidehAlassi SepidehAlassi Mar 23, 2020

Choose a reason for hiding this comment

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

the use case is here is more complex because the value of the FormControl is a string / label as long as the user types and an instance of ReadResource once the user selects from the list.

If I understand what you suggest correctly, you mean there should be a validator to check that a chosen option is an instance of ReadResource. However, in "update", "create", and "search" modes, the dropdown menu shows the options for the user to choose from. These options are always instances of ReadResource. The user does not have any other choice than choosing one item from the menu. Hence, an option selected by the user is always an instance of ReadResource. That would mean that such a double validation would not actually be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, in "update", "create", and "search" modes, the dropdown menu shows the options for the user to choose from. These options are always instances of ReadResource. The user does not have any other choice than choosing one item from the menu. Hence, an option selected by the user is always an instance of ReadResource. That would mean that such a double validation would not actually be necessary.

Yes, but as long as the user types, the FormControl's value is just a string (the label). So this is to make sure that the user selected an option from the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the "save" button is inactive unless an option is chosen from the dropdown menu.

Copy link
Contributor

@tobiasschweizer tobiasschweizer Mar 24, 2020

Choose a reason for hiding this comment

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

no, not yet. But you test it by directly adding the link value component to app.component.html. I think @mdelez will wok on a create component soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

or you can just try it out in your spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am trying out in spec for a while now. It would have been nice to have the component.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes! Now we can make it a user story :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@SepidehAlassi I can start to work on the create component tomorrow

@SepidehAlassi
Copy link
Contributor Author

SepidehAlassi commented Mar 25, 2020

@tobiasschweizer I have added the missing validator and tests for creation mode in case user enters a string instead of choosing an item from the dropdown menu.

@SepidehAlassi
Copy link
Contributor Author

@tobiasschweizer I think this PR can really be merged, oder?

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer I think this PR can really be merged, oder?

I think you are almost done, please see my comments.

@SepidehAlassi
Copy link
Contributor Author

@tobiasschweizer Can I merge this now?

// at least 3 characters are required
if ( typeof searchTerm === 'string' && searchTerm.length >= 3) {

this.knoraApiConnection.v2.search.doSearchByLabel(searchTerm, 0, {limitToResourceClass: this.restrictToResourceClass}).subscribe(
(response: ReadResource[]) => {
this.resources = response;
});
} else {
// clear selection
this.resources = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea of clearing the selection is good, what I meant is just this.resources = []

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Choose a reason for hiding this comment

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

looks good, thanks

I have just one last comment: #27 (review)

@Input() displayValue?: ReadLinkValue;
@Input() parentResource: ReadResource;
@Input() propIri: string;
resources: ReadResource[] = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tobiasschweizer I have already initialized it here to an empty array

Copy link
Contributor

Choose a reason for hiding this comment

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

I understood that your idea of clearing selection empties the list when the user types and the string is still too short. I think this idea is good.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is important when user selects a resource and then wants to change it again. Then the selection has to change to.

Copy link
Contributor

Choose a reason for hiding this comment

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

because I think otherwise you still get the old selection when the label string is shorter than 3 chars

Copy link
Contributor

Choose a reason for hiding this comment

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

but maybe I am wrong, I will leave this to you. Please proceed as you want.

@SepidehAlassi
Copy link
Contributor Author

SepidehAlassi commented Mar 25, 2020

looks good, thanks

I have just one last comment: #27 (review)

see my reply above

@SepidehAlassi SepidehAlassi merged commit 93649be into master Mar 25, 2020
@SepidehAlassi SepidehAlassi deleted the wip/14-linkValueComponent branch March 25, 2020 14:53
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.

Viewer Value Component: LinkValueComponent
4 participants