Skip to content

Show request details in popup modal#750

Open
shanavas786 wants to merge 2 commits into
raksha-life:masterfrom
shanavas786:req-summary
Open

Show request details in popup modal#750
shanavas786 wants to merge 2 commits into
raksha-life:masterfrom
shanavas786:req-summary

Conversation

@shanavas786

Copy link
Copy Markdown

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
Copy Markdown
Author

sample
modal

const iGps = 6
const iSummary = 7

window.onload = function() {

Copy link
Copy Markdown

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
Copy Markdown
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

Copy link
Copy Markdown

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
Copy Markdown
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

@realslimshanky realslimshanky left a comment

Copy link
Copy Markdown

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
Copy Markdown

@shanavas786 please resolve conflicts as well.

@shanavas786

Copy link
Copy Markdown
Author

done

@realslimshanky realslimshanky left a comment

Copy link
Copy Markdown

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