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

Sandcastle tweaks #1956

Merged
merged 9 commits into from
Jul 25, 2014
Merged

Sandcastle tweaks #1956

merged 9 commits into from
Jul 25, 2014

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jul 24, 2014

Added the below two functions to Sandcastle and made the relevant demos include them.

  1. Sandcastle.addDefaultToolbarButton executes and highlights the toolbar immediately, eliminating the need for breaking out an extra function to call (muddying the code in the process).
  2. When defined by a demo, Sandcastle.reset is called whenever a button or menu selection is main. This allows for the bookkeeping/cleanup code to be cleanly abstracted away from the example-specific code that the user is interested in.

mramato added 3 commits July 24, 2014 12:05
Sandcastle.addDefaultToolbarButton excutes and highlights the toolbar immediately, eliminating the need for breaking out an extra function to call (muddying the code in the process).
When defined by a demo, Sandcastle.reset is called whenever a button or menu selection is main.  This allows for the bookeeping/cleanup code to be cleanly abstracted away from the example-specific code that the user is interested in.
@emackey emackey mentioned this pull request Jul 24, 2014
@emackey
Copy link
Contributor

emackey commented Jul 25, 2014

This needs a merge from master now, and edits to the GeoJSON example.

@mramato
Copy link
Contributor Author

mramato commented Jul 25, 2014

Yep, doing it now. I also realized I need a change to the default/reset logic due to a problem exposed by the updated GeoJSON example.

Also changed logic to defer execution of the default action until loading is finsihed; this way we can run the reset function before the default action.
@mramato
Copy link
Contributor Author

mramato commented Jul 25, 2014

Okay, ready.

@emackey
Copy link
Contributor

emackey commented Jul 25, 2014

Got an idea, doesn't have to be for this PR, but let me know what you think:

var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;
var miscMenu = Sandcastle.addToolbarMenu('Misc Materials');

miscMenu.addDefaultOption('Rim Lighting', function() {
    // Cesium example code here
});

miscMenu.addOption('Water', function() {
    // Cesium example code here
});

Basically, by declaring menu(s) at the top, and then adding options individually to the menus, we can make the options get added similar to how buttons get added. This would avoid them needing .declare and .highlight, and the sample code for an option would be in the same place in the source where it gets added. It would remove the "lists of options" filling up space at the bottom of the example.

What do you think?

@mramato
Copy link
Contributor Author

mramato commented Jul 25, 2014

Great idea. Are you going to take a stab at it? If not I can probably do it over the weekend (in a different PR).

mramato added 3 commits July 25, 2014 12:01
…oolbar

Conflicts:
	Apps/Sandcastle/gallery/3D Models.html
…oolbar

Conflicts:
	Apps/Sandcastle/gallery/Billboards.html
	Apps/Sandcastle/gallery/Terrain.html
menu.onchange = onchange;
menu.onchange = function() {
window.Sandcastle.reset();
if (menu.selectedIndex > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not all menus have a header at index 0, for example the Billboards menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I improved the check so it doesn't matter now. This is ready.

emackey added a commit that referenced this pull request Jul 25, 2014
@emackey emackey merged commit d826791 into master Jul 25, 2014
@emackey emackey deleted the sandcastle-defaultToolbar branch July 25, 2014 18:34
@emackey
Copy link
Contributor

emackey commented Jul 25, 2014

@mramato I didn't get any time to work on the menu feature today. Let me know if you attack it this weekend, otherwise I'll try to carve out time next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants