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

Fix label filter not working #230

Merged
merged 2 commits into from
Feb 6, 2024
Merged

Fix label filter not working #230

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4be01f1
Update model value from `name` to `formattedName`
nknguyenhc Jan 22, 2024
12d591c
Update hidden label model
nknguyenhc Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/app/shared/filter-bar/label-filter-bar/label-filter-bar.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<mat-list-option
#option
*ngFor="let label of this.labels$ | async"
[value]="label.name"
[value]="label.formattedName"
[selected]="selectedLabelNames.includes(label.name)"
Copy link
Collaborator

@gycgabriel gycgabriel Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What about the other uses of label.name instead of formattedName, will there be any edge cases from changing only the value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value being changed to label.formattedName only affects the values in LabelFilterBarComponent::selectedLabelNames. It is only used internally, to update the value of the behaviour subject field LabelFilterBarComponent::selectedLabels. In LabelFilterBarComponent::simulateClick and LabelFilterBar::updateSelection methods:

this.selectedLabels.next(this.selectedLabelNames);

LabelFilterBarComponent::updateSelection value is an @Input field. When looking for places that this component is used, it is only used in FilterBarComponent, in filter-bar.component.html:

<mat-grid-tile class="label-filter-grid-tile" colspan="1">
  <app-label-filter-bar [selectedLabels]="this.labelFilter$" [hiddenLabels]="this.hiddenLabels$"></app-label-filter-bar>
</mat-grid-tile>

Hence any changes to LabelFilterBarComponent::selectedLabels will only cause a change to FilterBarComponent::labelFilter$. Looking at the places that this field is used, it is only used in FilterBarComponent::ngAfterViewInit, which subscribes to changes to this field:

    this.labelFilterSubscription = this.labelFilter$.subscribe((labels) => {
      this.dropdownFilter.labels = labels;
      this.applyDropdownFilter();
    });

On every change, the FilterBarComponent::dropdownFilter.labels is changed, and then applyDropdownFilter is called, where the filter in each of the views$ are updated to FilterBarComponent::dropdownFilter. Here, the views$ are the child components that are filterable. It is an @Input field. Looking at places where FilterBarComponent is used, only in issue-viewer.component.html:

<app-filter-bar [views$]="views" #filterbar></app-filter-bar>

The value of FilterBarComponent::views$ takes the value of IssueViewer::views. IssueViewer::views is a field that follows the value of IssueViewer::cardViews:

  ngAfterViewInit(): void {
    this.viewChange = this.cardViews.changes.subscribe((x) => this.views.next(x));
  }

Which in turn is a @ViewChildren component that is of type QueryList<CardViewComponent>! This means that any change to FilterBarComponent::dropdownFilter will cause a change to the filters of all CardViewComponents within IssueViewer.

TLDR, in short, the change of value from label.name to label.formattedName only means that all CardViewComponents filter by label.formattedName instead of label.name. Note that CardViewComponent has selector app-view-card, so to double check where this change will affect, paste the following code to the browser's console when opening the app:

document.querySelectorAll('app-card-view')

We see that the CardViewComponents are only the columns of the contributors of the current repo. This is the only component that takes the intended effect of the change.

I realised that the LabelFilterBarComponent::hiddenLabelNames needs a similar change. Similar to what I have explained above, the CardViewComponents are the only component that takes the intended effect of the change. However, looks like it only hide the labels. I'm not sure if this is the intended behaviour of the hiding feature, if it not, there needs to be a change to how the CardViewComponents make use of the filter.hiddenLabels.

image

After hiding the label difficulty.Easy:

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

However, looks like it only hide the labels. I'm not sure if this is the intended behaviour of the hiding feature, if it not, there needs to be a change to how the CardViewComponents make use of the filter.hiddenLabels.

To confirm, what did you think the hiding feature should do? We might want to reconsider how we present the features or change the feature if it doesn't make intuitive sense to the user

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 it should hide the issues & PRs that have the tag, instead of simply hide the tag from the page. I have posted a new issue on this at #240.

class="list-option"
[class.hidden]="filter(input.value, label.name)"
Expand Down
Loading