-
Notifications
You must be signed in to change notification settings - Fork 753
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
Add global filters #425
Add global filters #425
Conversation
6da3390
to
f0e6e48
Compare
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.
Looking really good! Amazing work here @joanagmaia
Left a minor comment, and will test try to test things locally before approving 👍
/> | ||
</div> | ||
</app-page-wrapper> | ||
<!-- Custom Report --> | ||
<div v-else class="max-w-5xl flex flex-grow mx-auto"> | ||
<app-report-grid-layout | ||
v-model="report" | ||
:is-public-view="true" |
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.
Was this hardcoded on purpose?
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, maybe the name itself is not the best but basically this boolean indicates that the report is being seen in the public view. Since this is the report-view-page-public.vue
component this is always the public view
Should I update the prop name to something else?
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.
Makes sense!
Should I update the prop name to something else?
No need, just wanted to double-check if was intended 👍
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.
Once again, great job 👏
Left a minor suggestion, but it should not be a blocker to merge
} | ||
|
||
const onTrackFilters = () => { | ||
window.analytics.track('Filter report', { |
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.
Maybe we should differentiate here that this is a template/default report (within the event name)
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.
Will do! Thanks for the suggestion
Changes proposed ✍️
widget-queries
to hold all template related queries logicreport-template-filters
component to handle the rendering and update logic of the filterScreenshots (front-end changes only)
Checklist ✅
Feature
,Enhancement
, orBug
.frontend/.env.dist.local
,frontend/.env.dist.composed
.backend/.env.dist.local
,backend/.env.dist.composed
.