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

Show request details in popup modal #750

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shanavas786
Copy link

Issue Reference

This PR addresses the Issue : Closes #626

Summarize

By handling request detail in request list page itself, we can reduce hits to request details api

@shanavas786
Copy link
Author

sample
modal

const iDate = 5
const iGps = 6
const iSummary = 7

window.onload = function() {

Choose a reason for hiding this comment

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

Since we use jQuery, change this window.onload to

$(window).on('load', function(){
})

Copy link
Author

Choose a reason for hiding this comment

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

Thanks,
But we can't do that because jquery script is loaded at the bottom of the page. using $ would through an error

Choose a reason for hiding this comment

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

Ah okay..

Then changing it to window.addEventListener would be good enough. I saw in other places window.onload is been used. It will cause the onload overridden from other places.

Copy link
Author

Choose a reason for hiding this comment

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

chaged to window.addEventListener, though overriding won't be an issue as only one template is loaded at a time

Copy link

@realslimshanky realslimshanky left a comment

Choose a reason for hiding this comment

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

LTGM. It would be better if you place CSS and JS in their respective files in static.

@realslimshanky
Copy link

@shanavas786 please resolve conflicts as well.

@shanavas786
Copy link
Author

done

Copy link

@realslimshanky realslimshanky left a comment

Choose a reason for hiding this comment

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

Please squash your commits. Rest LGTM.

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.

4 participants