-
Notifications
You must be signed in to change notification settings - Fork 4
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
gh-529 Add tree modal to catalogue item search screen #571
Conversation
@aaronforshaw This is just an idea, don't know how effective it will be. But I've been working on a branch in getSearchableResource(domain: string): SearchableResource => {
if (domain === 'dataModels') {
return this.resources.dataModel;
}
if (domain === 'terminiologies') {
return this.resources.terminology;
}
/// etc...
}
// ...
const resource: SearchableResource = getSearchableResource('dataModels');
resource.search(...); This is because there are 2 things which I don't like about the current
This may not be perfect though, I'm not sure about the Data Class search since that requires more parameters to locate the Data Class in the catalogue. |
contextId?: string; | ||
contextLabel?: string; | ||
contextParentId?: string; | ||
contextDataModelId?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to combine these into another interface type, since they are all related? e.g.
export interface CatalogueSearchContext {
domainType?: string;
id?: string;
label?: string;
parentId?: string;
dataModelId?: string;
}
export interface CatalogueSearchParameters {
context?: CatalogueSearchContext;
// etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this as yes, it seems neater, but then realised it takes a lot of work to pack and unpack the context parameters to and from the query string. So I ended up reverting to the original approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough
} | ||
|
||
openCatalogueItemSelectModal() { | ||
this.dialog.open(CatalogueItemSelectModalComponent, { }).afterClosed().subscribe((catalogueItem) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dialog with the model tree selection looks good. You might want to set a maxHeight
on the dialog when opening it though, it may be possible that the tree control grows big enough to push the dialog contents "Cancel" and "OK" off-screen so the user can't actually continue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what you mean, but the tree view seems to come with a vertical scroller in a fixed height container (I haven't figured out where the height is set).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, somehow the tree view height is restricted enough to not go beyond screen boundaries, I've seen that in action now.
@Injectable({ | ||
providedIn: 'root' | ||
}) | ||
export class ContentSearchHandlerService { | ||
|
||
constructor(public resources: MdmResourcesService, public validator: ValidatorService, public elementTypes: ElementTypesService) { } | ||
|
||
search(contextElement, searchText, limit, offset, | ||
search(contextElement: SearchContext, searchText, limit, offset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the ContentSearchHandlerService
is being modified, I think a lot of improvements could be made along the way:
- Types on parameters
- Correct return type
- Date validation is repetitive
- Actual search operation is repetitive - see my idea in the main conversation thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a bit nervous about modifying ContentSearchHandlerService
as it is used in several other parts of the UI. So I have added a new contextualSearch
method to CatalogueSearchService
. This new method uses your idea from the main thread, with special handling for searching a Data Class.
It seems that the backend search service for Data Classes only allows the root Data Class within a Data Model to be searched. So I have also added a doNotShowChildDataClasses
option to the tree, the stop a user from expanding the root data class.
@aaronforshaw Some more things I found while testing (don't know if you know these already):
The
|
@aaronforshaw Please ignore my comment on incorrect HTTP request URLs, I found the cause was down to my own local changes. |
a0f0521
to
2106465
Compare
f30484e
to
8e8555c
Compare
Regarding the issue of the tree not properly enabling the selection of ReferenceDataModel, Terminology and CodeSet, I have added ReferenceData to the list of elements accepted by the tree. Terminology and CodeSet can be added in the same way (see the line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks @aaronforshaw 👍
When I was testing I found a few bugs around the search, but I don't think they are related to your changes, I'll create separate issues for them.
Approved ✔️
Closes #529
contextualSearch
method toCatalogueSearchService
in order to limit the search to the item selected from the tree (see caveats in comments below).Observations which could be raised in separate tickets: