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

Case insensitive filter in group topics view #1774

Merged
merged 7 commits into from
Nov 7, 2023

Conversation

kszapsza
Copy link
Contributor

  • Filter in GroupTopicsListing view is case insensitive, so that we can find e.g. MyFavoriteTopic by typing myfavoritetopic. Same for GroupListing (though generally group names are lowercase either way).
  • Fix Unresolved function or method toBeInTheDocument() warnings in hermes-console-vue tests (add jest to globals in tsconfigs).

fixes `Unresolved function or method toBeInTheDocument()` warnings in `hermes-console-vue` tests
Copy link
Contributor

@szczygiel-m szczygiel-m left a comment

Choose a reason for hiding this comment

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

Can You also apply this change to other listings? E.g subscription search in topicView

@@ -17,5 +17,5 @@ export interface InconsistentSubscription {

export interface InconsistentMedata {
datacenter: string;
content: string | undefined;
content?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we have content: string | undefined, we can have undefined value, but the key content has to be present in the object, so in stubs we have to write: content: undefined (otherwise we have a type error).

In case of content?: string, we can both have object with key content with value undefined provided explicitly, but we also can omit this key completely (e.g. in test data, the value of absent key will still be undefined in JS).

@@ -25,27 +25,30 @@

const subscriptionItems = computed(() =>
props.subscriptions
?.filter((subscription) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous approach in which we mapped either to object (name/color/statusText/href) or to null and then filtered with .filter(Boolean) was problematic for TS type system. Though the operation filtered out null objects, IntelliJ still complained in Vue template below, that subscriptionItems elements can be possibly null.

We could introduce an interface:

interface SubscriptionListItem {
  name: string;
  color: string;
  statusText: State;
  href: string;
} 

and then change .filter(Boolean) filter to the so-called TypeScript type predicate:

.filter((subscription): subscription is SubscriptionListItem => subscription !== null)

then TS type system would know, that null values possibly introduced in map above were filtered out.

But IMHO it is cleaner to simply perform subscription filtering before map, as I did here

@piotrrzysko piotrrzysko merged commit ff1614b into allegro:master Nov 7, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants