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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

added iframe embed #13

Merged
merged 1 commit into from Mar 12, 2017

Conversation

Projects
None yet
3 participants
@Overtorment
Contributor

Overtorment commented Mar 4, 2017

Was able to make a PR without even managing to run the project and never using Go before 馃ぃ
Needs testing and feedback ofcourse. Not sure if I cut too much from html template.
Note, the Powered by added :-)

@karanjthakkar

This comment has been minimized.

Show comment
Hide comment
@karanjthakkar

karanjthakkar Mar 5, 2017

Owner

@Overtorment Thanks for taking this up!

Since you're only stripping down parts of the page, I think that a good idea would be to add a boolean to the Response type IsEmbeddable or something similar. And then use if else conditions within the user template to add/strip some parts.

Sections that I think are not needed in the embedded markup:

  1. Username
  2. Tweet button
  3. Last Synced text

Let me know if you have any doubts :)

Owner

karanjthakkar commented Mar 5, 2017

@Overtorment Thanks for taking this up!

Since you're only stripping down parts of the page, I think that a good idea would be to add a boolean to the Response type IsEmbeddable or something similar. And then use if else conditions within the user template to add/strip some parts.

Sections that I think are not needed in the embedded markup:

  1. Username
  2. Tweet button
  3. Last Synced text

Let me know if you have any doubts :)

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Mar 5, 2017

Contributor

Reworked as proposed.
Not sure if GA code and metatags should be in iframed document tho.
Looks nice on my machine:

image

Contributor

Overtorment commented Mar 5, 2017

Reworked as proposed.
Not sure if GA code and metatags should be in iframed document tho.
Looks nice on my machine:

image

@karanjthakkar

Apart from these two changes, LGTM :)

Show outdated Hide outdated templates/user.html Outdated
@@ -187,6 +187,8 @@ func main() {
go SaveDataAsJson(data, username)
}
resultData.ResponseType = responseType

This comment has been minimized.

@karanjthakkar

karanjthakkar Mar 9, 2017

Owner

Not super important, but can you default responseType to default if it is not one of json or iframe?

@karanjthakkar

karanjthakkar Mar 9, 2017

Owner

Not super important, but can you default responseType to default if it is not one of json or iframe?

This comment has been minimized.

@Overtorment

Overtorment Mar 9, 2017

Contributor

Not sure how to do that? This is my first time messing with GO code.

@Overtorment

Overtorment Mar 9, 2017

Contributor

Not sure how to do that? This is my first time messing with GO code.

This comment has been minimized.

@karanjthakkar

karanjthakkar Mar 9, 2017

Owner

Ahh, no problem. Lets skip it for now.

@karanjthakkar

karanjthakkar Mar 9, 2017

Owner

Ahh, no problem. Lets skip it for now.

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Mar 9, 2017

Contributor

@karanjthakkar added indentation

Contributor

Overtorment commented Mar 9, 2017

@karanjthakkar added indentation

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Mar 9, 2017

Contributor

rebased against master to keep git log tidy

Contributor

Overtorment commented Mar 9, 2017

rebased against master to keep git log tidy

Show outdated Hide outdated README.md Outdated

@karanjthakkar karanjthakkar merged commit f899c54 into karanjthakkar:master Mar 12, 2017

@karanjthakkar

This comment has been minimized.

Show comment
Hide comment
@karanjthakkar

karanjthakkar Mar 12, 2017

Owner

Thanks for the work @Overtorment! Sorry about the confusion with template separation. I've added it and merged here: bf4fe1c The only modification I made was removing the Made By section since it was overlapping the repo list behind.

Owner

karanjthakkar commented Mar 12, 2017

Thanks for the work @Overtorment! Sorry about the confusion with template separation. I've added it and merged here: bf4fe1c The only modification I made was removing the Made By section since it was overlapping the repo list behind.

@Overtorment Overtorment deleted the Overtorment:add-iframe-embed branch Mar 12, 2017

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