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.

Sign up for GitHub

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

Jump to bottom

[ACS-5266] Advanced Search - New component for Category facet #8764

Merged
merged 9 commits into from
Jul 21, 2023
Merged

[ACS-5266] Advanced Search - New component for Category facet #8764

merged 9 commits into from
Jul 21, 2023

Conversation

nikita-web-ua
Copy link
Contributor

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation
  • Other... Please describe:

What is the current behaviour? (You can also link to an open issue here)

https://alfresco.atlassian.net/browse/ACS-5266

What is the new behaviour?

The same component as for Tags and Locations is reused to construct a new Category filter.

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@nikita-web-ua nikita-web-ua marked this pull request as ready for review July 17, 2023 11:03
@DenysVuika
Copy link
Contributor

Notes for reviewers: while this might look like a breaking change (the attribute name change), the component is instantiated dynamically via the search facet configuration, so should be fine.

MichalKinas
MichalKinas reviewed Jul 17, 2023
View reviewed changes
...arch/components/search-chip-autocomplete-input/search-chip-autocomplete-input.component.html
@@ -4,8 +4,14 @@
class="adf-option-chips"
*ngFor="let option of selectedOptions"
(removed)="remove(option)">
<span>{{option}}</span>
<button matChipRemove class="adf-option-chips-delete-button" [attr.aria-label]="('SEARCH.FILTER.BUTTONS.REMOVE' | translate) + ' ' + option">
<span [matTooltip]="'SEARCH.RESULTS.WILL_CONTAIN' | translate:{searchTerm: option.fullPath}"
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 it should take fullPath and if it's not present it should display value

MichalKinas
MichalKinas reviewed Jul 17, 2023
View reviewed changes
...arch/components/search-chip-autocomplete-input/search-chip-autocomplete-input.component.html
<span>{{option}}</span>
<button matChipRemove class="adf-option-chips-delete-button" [attr.aria-label]="('SEARCH.FILTER.BUTTONS.REMOVE' | translate) + ' ' + option">
<span [matTooltip]="'SEARCH.RESULTS.WILL_CONTAIN' | translate:{searchTerm: option.fullPath}"
[matTooltipDisabled]="!option.fullPath" [matTooltipShowDelay]="tooltipShowDelay">
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 we need to consider if we want to display tooltip when there is only value provided since it may happen that it will be too long to be displayed as a whole

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 thought about it as well, not sure if we need tooltips for other filters though. In the ticket for Tags & Location, there is no info about tooltips.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but after we release this, customers might use this component to define filters on some custom properties as well and we can't predict if they will contain longer values. But on the other hand they can always use the fullPath so I think we can leave it as it for now and in case of bugs modify later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tooltips for autocomplete in case of long values as long values shrink there, for chips we need tooltips only for Categories to show the full path.

MichalKinas
MichalKinas reviewed Jul 17, 2023
View reviewed changes
...arch/components/search-chip-autocomplete-input/search-chip-autocomplete-input.component.html
{{ option.value }}
</span>
<button matChipRemove class="adf-option-chips-delete-button" [matTooltipDisabled]="!option.fullPath"
[matTooltip]="('SEARCH.FILTER.BUTTONS.REMOVE' | translate) + ' \'' + option.fullPath + '\''"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for tooltip

MichalKinas
MichalKinas reviewed Jul 17, 2023
View reviewed changes
...arch/components/search-chip-autocomplete-input/search-chip-autocomplete-input.component.html
*ngFor="let option of filteredOptions"
[matTooltip]="'SEARCH.RESULTS.WILL_CONTAIN' | translate:{searchTerm: option.fullPath}"
[value]="option"
[matTooltipDisabled]="!option.fullPath" matTooltipPosition="right"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for tooltip

AleksanderSklorz
AleksanderSklorz reviewed Jul 18, 2023
View reviewed changes
...search/components/search-chip-autocomplete-input/search-chip-autocomplete-input.component.ts
this.formCtrl.valueChanges
.pipe(
startWith(''),
debounce((value: string) => (value ? timer(300) : EMPTY)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you put timer because you do searching for categories when you type right? I'm wondering if that should not be optional because what if anyone has hardcoded options so no need for extra delays during typing?

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 think that it should not be a problem, as it is pretty small delay and it makes dropdown update smoother then on every letter

@nikita-web-ua nikita-web-ua force-pushed the dev-mmaliarchuk/ACS-5266-new-component-for-category-facet branch from 294d55e to 2ed7b20 Compare July 19, 2023 13:16
@AleksanderSklorz
Copy link
Contributor

@nikita-web-ua I see some errors in console when I open search:
image

@nikita-web-ua
Copy link
Contributor Author

@nikita-web-ua I see some errors in console when I open search: image

How do you get them? I don't see any

@AleksanderSklorz
Copy link
Contributor

@nikita-web-ua I see some errors in console when I open search: image

How do you get them? I don't see any

Hmm @nikita-web-ua can you try to link with my ACA configuration PR? It may be a case that it is related with Properties filter. I had not that error earlier but on your branch I'm getting this with my configuration. Here is my PR: Alfresco/alfresco-content-app#3347

@AleksanderSklorz
Copy link
Contributor

@nikita-web-ua I see some errors in console when I open search: image

How do you get them? I don't see any

Hmm @nikita-web-ua can you try to link with my ACA configuration PR? It may be a case that it is related with Properties filter. I had not that error earlier but on your branch I'm getting this with my configuration. Here is my PR: Alfresco/alfresco-content-app#3347

@nikita-web-ua basically in that configuration I preconfigured possible extensions so it may be reason why you don't see error - because extensions are not loaded without my configuration

@AleksanderSklorz
Copy link
Contributor

@nikita-web-ua FYI my ACA PR with configuration is merged now so you can just pull changes in ACA

@AleksanderSklorz
Copy link
Contributor

@nikita-web-ua FYI my ACA PR with configuration is merged now so you can just pull changes in ACA

We discussed that on slack, probably I had some missing configuration in ACA

@nikita-web-ua nikita-web-ua force-pushed the dev-mmaliarchuk/ACS-5266-new-component-for-category-facet branch from dd5e5d5 to 2d5bb47 Compare July 20, 2023 11:35
@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@nikita-web-ua nikita-web-ua merged commit 2a4507d into develop Jul 21, 2023
33 checks passed
@nikita-web-ua nikita-web-ua deleted the dev-mmaliarchuk/ACS-5266-new-component-for-category-facet branch July 21, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichalKinas MichalKinas MichalKinas left review comments

@DenysVuika DenysVuika DenysVuika approved these changes

@AleksanderSklorz AleksanderSklorz AleksanderSklorz approved these changes

@eromano eromano Awaiting requested review from eromano eromano is a code owner

@popovicsandras popovicsandras Awaiting requested review from popovicsandras popovicsandras is a code owner

Assignees
No one assigned
Labels
A/N BDU
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nikita-web-ua @DenysVuika @AleksanderSklorz @MichalKinas