Skip to content
This repository was archived by the owner on Dec 27, 2023. It is now read-only.

JSON Objects communication - #179

Open
bhavul wants to merge 10 commits into
privly:masterfrom
bhavul:InjectableFailPassive
Open

JSON Objects communication#179
bhavul wants to merge 10 commits into
privly:masterfrom
bhavul:InjectableFailPassive

Conversation

@bhavul

@bhavul bhavul commented Mar 2, 2015

Copy link
Copy Markdown
Contributor

The sending of resize message works. Hosting page sends a resize JSON object with the details of the iframe and the height of the content of iframe. privly.js recieves it and processes on it.

@bhavul
bhavul force-pushed the InjectableFailPassive branch from 4cd9636 to eaab46a Compare March 11, 2015 20:30
@bhavul

bhavul commented Mar 11, 2015

Copy link
Copy Markdown
Contributor Author

@smcgregor check host_pages_integration.js
I know I didn't have to add the code I did. But, that's just to show how I've checked the working of all 4 functions as of now. I'll remove them. Just wanted you to have a look at them once. I've commented them out. L39 to L52.

@bhavul
bhavul force-pushed the InjectableFailPassive branch from eaab46a to 43ee020 Compare March 21, 2015 01:18
@bhavul

bhavul commented Mar 21, 2015

Copy link
Copy Markdown
Contributor Author

@smcgregor Added other methods too. Only adding tests remaining now, for this and privly.js file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These functions are largely duplicated 3 times. Maybe one helper function that is called with three different params?

@smcgregor

Copy link
Copy Markdown
Member

Are you still working on tests for this?

@bhavul

bhavul commented Mar 30, 2015

Copy link
Copy Markdown
Contributor Author

@smcgregor I'll do the helper function part and yeah, I had started working on the unit tests. I'll add those too in the next 3-4 days.

@smcgregor

Copy link
Copy Markdown
Member

Cool. This will be a great feature to have :)

@bhavul
bhavul force-pushed the InjectableFailPassive branch from 43ee020 to f3d951d Compare April 1, 2015 15:02
@smcgregor

Copy link
Copy Markdown
Member

This is still marked as "Needs Work." Is there something I am waiting on at this point, or should I be reviewing this PR?

@bhavul

bhavul commented Apr 19, 2015

Copy link
Copy Markdown
Contributor Author

@smcgregor Yes, just give me few more hours. I had got stuck at some silly place in integration testing and then have been a bit away due to academics. But, I'll add integration testing(it's almost done) and then leave you another message here to review it.

@bhavul

bhavul commented Apr 23, 2015

Copy link
Copy Markdown
Contributor Author

@smcgregor Just review it once.

Also, I have a query about integration testing file. As far as I've understood other such ruby and capybara files, they pseudo-act like users. Click on buttons, opens navigation and so on to check how the whole system works.
I've introduced an API here, which will be used by privly app developer. So, there's no such user action right now that can trigger showing/hiding/removing injected links. What should then be included and tested in an integration testing file for this?

@smcgregor

Copy link
Copy Markdown
Member

I think you might be limited to unit testing until an app implements the API.

@bhavul

bhavul commented Apr 25, 2015

Copy link
Copy Markdown
Contributor Author

I've added unit testing already. You can then review, I think.
Shoot, i should have then asked you to review this way way before. It's
been weeks since I completed unit testing.
On Apr 25, 2015 4:11 AM, "Sean McGregor" notifications@github.com wrote:

I think you might be limited to unit testing until an app implements the
API.


Reply to this email directly or view it on GitHub
#179 (comment)
.

@smcgregor

Copy link
Copy Markdown
Member

I wrote a shared messaging library that @summerwish abstracted so each of the scripting contexts could message each other across Jetpack/Chrome/Safari. The library should make it easier to integrate these changes across all platforms.

Sorry it has taken me so long to update you here. This has been noted in my email for 3 months. I think it would be best to test and merge this into develop once the context_messenger.js lands in develop.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants