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

Import and export Sandcastle examples using Gists #3795

Merged
merged 20 commits into from Apr 12, 2016

Conversation

Projects
None yet
4 participants
@TomPed
Copy link
Contributor

commented Mar 31, 2016

Fixes #3702, #2152

Things to do:

  • Log errors to the sandcastle console when importing gists
  • Fix share. Right now when you click share you get the id of a gist but I want this to be a link that will open up sandcastle and paste the code in from the gist with that id
  • Fix icons
  • Fix textbox
@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

  • Update CHANGES.md
  • Do you need icons for import/export?
@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

@TomPed please review #2152. We may be able to close that and #3702 with this PR.

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2016

These are the icons that I am using right now:

screen shot 2016-04-01 at 9 58 07 am

What do you think?

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

Can we use the standard share icon? @ggetz can advise on what we used for the website.

And perhaps use the GitHub icon for import gist if their terms of use allow it.

@ggetz

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2016

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2016

Okay I will try to find a dijit icon similar since that seems to be the theme of sandcastle.

TomPed added some commits Apr 1, 2016

@TomPed TomPed changed the title Import working, almost done with sharing Import and export Sandcastle examples using Gists Apr 1, 2016

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2016

I've made some changes. All I need to do now is to not auto run code on page load. If you click import gist the code does not auto run, but if you enter a url with the query parameter for a gist the code will run automatically, which could cause security issues. I'm trying to figure out a fix for this.

TomPed added some commits Apr 4, 2016

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2016

Fixed auto run problem. @pjcozzi one last thing I think. We discussed having a header when we share an example in the GitHub Gist (like the Cesium version and stuff like that). What if someone imports a Gist, with the header. Tweaks the code and then creates a new gist. That gist will have a double header. Is it worth fixing this (with an easy if) or should we not include the header? If we want the header what should it include besides the Cesium version?

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2016

History works for gists, but not for other sandcastle examples. Working on that fix now.

@@ -119,3 +119,17 @@ The 16 x 16px icons in these sprites are action and object type images which can
.dijitRtl .dijitDisabled .dijitIconError {
background-image: url('images/commonIconsObjActDisabled_rtl.png'); /* Contains both object and action icons as a sprite image for the disabled state. These would be used by buttons and menus. */
}

.gitHubIcon {

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 5, 2016

Member

This file is part of Dojo, which is a third-party library. Do not modify it. There's probably a .css file in the Apps/Sandcastle directory you can use. If not, @emackey will know.

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

Merge in master. There is probably a trivial conflict in CHANGES.md.

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

History works for gists, but not for other sandcastle examples. Working on that fix now.

I don't know what you mean.

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

We discussed having a header when we share an example in the GitHub Gist (like the Cesium version and stuff like that). What if someone imports a Gist, with the header. Tweaks the code and then creates a new gist. That gist will have a double header. Is it worth fixing this (with an easy if) or should we not include the header? If we want the header what should it include besides the Cesium version?

It would be OK to have a running history log, but the gist is already backed by a git repo. I was thinking to include the Cesium version and save date/time, which could be useful if there is a breaking API change in Cesium.

However, why not start with no header at all, and then let's see if we actually need it.

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2016

I am looking into if we need to updated https://github.com/AnalyticalGraphicsInc/cesium/blob/master/LICENSE.md#example-applications. And do so if needed.

I don't know what you mean.

History is not working correctly. This has been an issue for a little while now. I think the best thing to do would be to create a new HTML file and have the link to the gist open that. I will get this change out before lunch so you can review.

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2016

From the font awesome website.

Attribution is no longer required as of Font Awesome 3.0 but is much appreciated: "Font Awesome by Dave Gandy - http://fontawesome.io".

As for the GitHub icon I don't think we need to document that either. From the GitHub page.

Use the Octocat or GitHub logo to advertise that your product has built-in GitHub integration

The above is permitted

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

Let's still include them just for attribution.

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2016

I have browser history working 90%. I will be talking with @emackey tomorrow to discuss my approach and help me out.

TomPed added some commits Apr 6, 2016

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2016

I now believe bowser history is 100% working. I think this is ready for a look!

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 7, 2016

@emackey @shunter do you want to look at this at all?

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2016

As discussed offline with @emackey. Right now when you navigate to a exported sandcastle example the code does not auto run for security reasons. However since cesiumjs.org does not hold any user account info we think that we can allow the code from the gist to auto run. Does anyone anyone else see a security issue here?

@mramato

This comment has been minimized.

Copy link
Member

commented Apr 8, 2016

I can't think of a good reason not to auto-run the code.

Also, I discussed with @TomPed a bit offline, but I think the share button should probably pop up a modal with the link filed in rather than have that weird empty text field it has right now. The modal could also let the user know that further edits to the code will not be reflected in the link (which is non-obvious right now).

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2016

I've revamped the importing. Check it out.

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

If there are no security concerns, auto-run is OK with me.

@TomPed is there anything else before we merge this?

Does anyone else want to review this?

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

I will make that change now. I will also make some changes with the error handling. And do one final "user test".

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

Made the changes that I wanted. If anyone else would take a look, that'd be great!

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

Can the "Share" dialog appear right under the toolbar button instead of in the center of the page?

Have you tried the share workflow for a few different apps? I know we discussed not wanting to create the GitHub gist when they first click the toolbar button, but this feels awkward to me. For example, shouldn't the "Get Link" button grey or go away once the link is shown? Should we auto select-all? Should just clicking in the text box create the link? What is simple and painful for the user?

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

Fun idea: could be cool to write a Chrome plugin that can open a GitHub gist in Sandcastle. Total overkill right now.

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

Let's not add it yet, but I wonder if long-term, we'll want a link to the GitHub gist from Sandcastle, e.g., https://gist.github.com/anonymous/7d67199111d68c6a6eceef8d0369192e, for comments and revision history.

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

Did you test sharing when your offline? Is there a reasonable error message?

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

Works great! Just those comments.

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

Okay so I've changed how sharing works. I think that this is very easy to use and similar to how a few websites share. Also you will notice if you don't change code after sharing and try to share again the same link will be in the textbox. @pjcozzi what do you think?

@TomPed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

Made error messages reasonable when offline.

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

This is all good with me.

Does anyone else want to review before I merge?

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

Thanks again @TomPed!

@pjcozzi pjcozzi merged commit 55d1609 into AnalyticalGraphicsInc:master Apr 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pjcozzi pjcozzi deleted the TomPed:import-export-sandcastle-gist branch Apr 12, 2016

@pjcozzi pjcozzi referenced this pull request Apr 18, 2016

Closed

Sandcastle/Gist interop #2152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.