-
Notifications
You must be signed in to change notification settings - Fork 149
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
[ACS-4130] Added autocomplete to folder rules 'Has Category' condition #3464
[ACS-4130] Added autocomplete to folder rules 'Has Category' condition #3464
Conversation
0110d50
to
f9aaf64
Compare
import { MatProgressSpinnerModule } from '@angular/material/progress-spinner'; | ||
import { CategoryEntry } from '@alfresco/js-api'; | ||
|
||
class AutoCompleteOption { |
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.
Do we need class here, can be an interface?
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.
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.
Yes you can use interface anywhere else and then in lines that you've mentioned you can just create an object like const option: AutoCompleteOption = { displayLabel, value }
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.
Updated AutoCompleteOption to be an interface instead of a class
...s/aca-content/folder-rules/src/rule-details/conditions/rule-simple-condition.ui-component.ts
Outdated
Show resolved
Hide resolved
...-content/folder-rules/src/rule-details/conditions/rule-simple-condition.ui-component.spec.ts
Outdated
Show resolved
Hide resolved
Good catch, I am getting these warnings on the develop branch as well. Also, the warning points towards rule-composite-condition.ui-component.html, line num 12 and line num 21. These are the disabled properties of UI element for the IF/NOT IF dropdown inside rule conditions. Acoording to the warning, we should set the disabled property via typescript instead of inside html
|
...-content/folder-rules/src/rule-details/conditions/rule-simple-condition.ui-component.spec.ts
Outdated
Show resolved
Hide resolved
...s/aca-content/folder-rules/src/rule-details/conditions/rule-simple-condition.ui-component.ts
Outdated
Show resolved
Hide resolved
...s/aca-content/folder-rules/src/rule-details/conditions/rule-simple-condition.ui-component.ts
Outdated
Show resolved
Hide resolved
...s/aca-content/folder-rules/src/rule-details/conditions/rule-simple-condition.ui-component.ts
Outdated
Show resolved
Hide resolved
...s/aca-content/folder-rules/src/rule-details/conditions/rule-simple-condition.ui-component.ts
Outdated
Show resolved
Hide resolved
...s/aca-content/folder-rules/src/rule-details/conditions/rule-simple-condition.ui-component.ts
Outdated
Show resolved
Hide resolved
[matAutocomplete]="auto" | ||
formControlName="parameter" | ||
(focusout)="autoSelectValidOption()" | ||
data-automation-id="auto-complete-input-field" |
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.
Can we unify spelling? Here you use auto-complete
and then autocomplete
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 reason why I was using auto-complete is because in the template, its camel-cased as autoComplete. Regardless, I've changed all occurences of autocomplete with auto-complete (except for references to the angular material component)
import { MatProgressSpinnerModule } from '@angular/material/progress-spinner'; | ||
import { CategoryEntry } from '@alfresco/js-api'; | ||
|
||
class AutoCompleteOption { |
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.
Yes you can use interface anywhere else and then in lines that you've mentioned you can just create an object like const option: AutoCompleteOption = { displayLabel, value }
.subscribe((existingCategoriesResult) => { | ||
this.showLoadingSpinner = false; | ||
const options: AutoCompleteOption[] = existingCategoriesResult?.list?.entries?.map((rowEntry) => { | ||
const option = new AutoCompleteOption(); |
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.
Looks similar to the code in line 155, maybe we can move autocomplete option mapping to a separate helper function?
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 thought of making a single helper function as well, but in the 2 codes, there is some slight difference in the object we are dealing with. One has a type of CategoryEntry, and the other has a type of ResultSetRowEntry. This makes a difference in how we fetch the complete path for the display label.
For Category entry, we do
category.entry.path.split('/').splice(3).join('/')
For ResultSetRowEntry, we do
rowEntry.entry.path.name.split('/').splice(3).join('/')
I've made a helper function with the 2 types shown below. If this looks fine, then I can push the code and we can go ahead with it
buildAutoCompleteOptionFromCategory(category: CategoryEntry | ResultSetRowEntry): AutoCompleteOption {
let path = '';
if (category instanceof CategoryEntry) {
path = category.entry.path.split('/').splice(3).join('/');
} else if (category instanceof ResultSetRowEntry) {
path = category.entry.path.name.split('/').splice(3).join('/');
}
const option: AutoCompleteOption = {
value: category.entry.id,
displayLabel: path ? `${path}/${category.entry.name}` : category.entry.name
};
return option;
}
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 think that you could also build this function in a bit different way. Instead of passing the whole objects you could just pass id
, path
and name
. Then you could use the same logic to build an option for both objects and the difference would just be in the function calls in the right places
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.
Updated the buildAutoCompleteOption method. It now takes in 3 arguments, categoryId, categoryPath, and categoryName, and generates an autocomplete option with the correct path accordingly
…Has Category rule condition. Options are now fetched as soon as user selected 'Has Category' option
…sting rule with has category option selected
…s Category' condition
…e dropdown when user focuses out of autocomplete input field
… interface. Changed occurences of autocomplete with auto-complete. Removed/Added types
…ilt using a single common helper method
32201b6
to
4f0f596
Compare
#3464) * [ACS-4130] Added autocomplete for 'Has Category' option in manage rules * [ACS-4130] Added loading spinner and 'No options found' template for Has Category rule condition. Options are now fetched as soon as user selected 'Has Category' option * [ACS-4130] Added code to fetch category name when viewing/editing existing rule with has category option selected * [ACS-4130] Resolved issues related to editing existing rules with 'Has Category' condition * [ACS-4130] Added safety checks and minor code refactoring * [ACS-4130] Added unit tests for new autocomplete functionality * [ACS-4130] Added feature to auto select first option from autocomplete dropdown when user focuses out of autocomplete input field * [ACS-4130] Minor code refactoring. Moved constants from global scope to local scope * [ACS-4130] Moved mock data to conditions.mock.ts. Removed redundant return type * [ACS-4130] Resolved PR review comments - AutoCompleteOption is now an interface. Changed occurences of autocomplete with auto-complete. Removed/Added types * [ACS-4130] Resolved PR review comments - AutoCompleteOption is now built using a single common helper method * [ACS-4130] Added missed types
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
User was unable to create a rule with the 'Has Category' condition, because the UI was passing in category name instead of category ID to the API. Additionally, there was no way to know which categories currently exist in the system
What is the new behaviour?
Added an autocomplete component to the 'Has Category' rule condition to provide user with a list of currently created categories. Also added code to pass in the category ID in place of category name to the API
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: