-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: Add debounce on search field and remove included resources #3646
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
Conversation
app/routes/admin/events/list.js
Outdated
let queryString = { | ||
get_trashed : true, | ||
include : 'tickets,sessions,speakers,owner,organizers,coorganizers,track-organizers,registrars,moderators', | ||
include : 'general-statistics,owner,organizers,coorganizers,track-organizers,registrars,moderators', |
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.
@iamareebjamal You will need to make this change in routes/events/list
too
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.
Also, Is the general-statistics
needed ?
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.
You will need to make this change in routes/events/list too
Where is this used?
OK. own events.
Also, Is the general-statistics needed ?
Yes, sessions and speaker counts are shown using general statistics
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, sessions and speaker counts are shown using general statistics
Speaker and Session Statistics are getting shown without using the general-statistics
?
Removed general statistics for now because it is custom endpoint on server and not linked correctly. Tried but failed to link it to event correctly, so, will handle later/ |
@prateekj117 @kushthedude Please review |
cellComponent : 'ui-table/cell/cell-speakers-dashboard', | ||
width : 90 | ||
}, | ||
{ |
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.
@iamareebjamal I think we should comment it out, As ticket statistics
have played a crucial role in our tables. We will need it once the DOS issues are resolved?
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 git repository, not a college project in a pen drive that something which is needed in the future needs to be commented out instead of deleting. It has the entire history of changes in the repo, any deleted thing can be recovered.
We will need it once the DOS issues are resolved
Then it'll add the DOS issue again. When is it a good idea to fetch unbounded one to many relationships of a list of objects? Answer - Never
If it'll need to be added, it'll be completely reimplemented and definitely not work like it was right now
It looks good then.
…On Fri, 22 Nov, 2019, 02:12 Areeb Jamal, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/controllers/admin/events/list.js
<#3646 (comment)>
:
> @@ -75,11 +75,6 @@ export default class extends Controller.extend(EmberTableControllerMixin) {
cellComponent : 'ui-table/cell/cell-speakers-dashboard',
width : 90
},
- {
This is a git repository, not a college project in a pen drive that
something which is needed in the future needs to be commented out instead
of deleting. It has the entire history of changes in the repo, any deleted
thing can be recovered.
We will need it once the DOS issues are resolved
Then it'll add the DOS issue again. When is it a good idea to fetch
unbounded one to many relationships of a list of objects? Answer - Never
If it'll need to be added, it'll be completely reimplemented
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3646?email_source=notifications&email_token=AKQMTLXL6LACUX6EIWETLVDQU3XENA5CNFSM4JP2D4IKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMSLTAI#discussion_r349307789>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKQMTLQPOCYCD7RQSI2R6LDQU3XENANCNFSM4JP2D4IA>
.
|
Fixes #3532 and Partial fix for #3642