-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat(web): activity feed fill the filters on chart bar click #2705
Conversation
NV-1611 Advanced Filters for activity feed
Created via Raycast |
templates: string[]; | ||
|
||
@ApiProperty({ | ||
enum: ChannelTypeEnum, | ||
isArray: true, | ||
}) | ||
channels: ChannelTypeEnum[]; |
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.
extended the graph stats response with template ids and channels
import { NotificationTemplateEntity, SubscriberRepository } from '@novu/dal'; | ||
import { ChannelTypeEnum } from '@novu/shared'; | ||
|
||
describe('Get activity feed graph stats - /notifications/graph/stats (GET)', async () => { |
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.
added tests for /notifications/graph/stats
endpoint
@@ -54,4 +54,46 @@ describe('Activity Feed Screen', function () { | |||
cy.get('.mantine-MultiSelect-item').contains('SMS').click(); | |||
cy.getByTestId('email-step').should('have.length', 0); | |||
}); | |||
|
|||
it('should show the clear filters button when template is selected', 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.
e2e tests for the filters clear button
@@ -76,6 +76,7 @@ | |||
"less": "^4.1.0", | |||
"lodash.capitalize": "^4.2.1", | |||
"lodash.clonedeep": "^4.5.0", | |||
"lodash.debounce": "^4.0.8", |
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'm using the debounce logic for the search and transaction id inputs... more about it later
the size of this lib is ~1kB gzip
search: filters?.search !== '' ? filters?.search : undefined, | ||
transactionId: filters?.transactionId !== '' ? filters?.transactionId : undefined, |
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 search/transaction id fields have empty strings do not include them in query params in the request
const onClick = (event) => { | ||
const [barItem] = getElementAtEvent(chartRef.current as any, event); | ||
if (barItem) { | ||
const { datasetIndex } = barItem; | ||
const item = data.datasets[datasetIndex].data[barItem.index]; | ||
|
||
onBarClick(item); | ||
} | ||
}; |
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.
when the chart bar is clicked retrieve it's data item...
it looks like this:
{
_id: '2023-01-01',
count: 4,
templates: [],
channels: []
}
parsing: { | ||
xAxisKey: 'dateLabel', | ||
yAxisKey: 'count', | ||
}, |
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.
changed this to be like this because before the "data item" was the count, now it's the object (shown in the above comment)
onHover: (event, el) => { | ||
if (el.length > 0) { | ||
event.native.target.style.cursor = 'pointer'; | ||
} else { | ||
event.native.target.style.cursor = 'default'; | ||
} | ||
}, |
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 is a hover trick, basically, the chart is a canvas, so there is no easy way of saying that on the bar hover it should show the cursor pointer, but we have this callback which is fired when hovering over the chart and if there are elements we do change the cursor style
@@ -114,6 +114,7 @@ export class NotificationRepository extends BaseRepository<EnforceEnvironmentQue | |||
_environmentId: new Types.ObjectId(environmentId), | |||
}, | |||
}, | |||
{ $unwind: '$channels' }, |
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 channels is an array, we should unwind it to be able to pass all items to the set
templates: { $addToSet: '$_templateId' }, | ||
channels: { $addToSet: '$channels' }, |
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.
templates and channels sets
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.
Amazing Work!
What change does this PR introduce?
Activity Feed Advanced Filters:
Why was this change needed?
Activity Feed Advanced Filters feature
Other information (Screenshots)
Screen.Recording.2023-02-08.at.10.49.15.mov