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

Gallery::Imagery Layers Split #5948

Merged
merged 7 commits into from
Nov 2, 2017
Merged

Gallery::Imagery Layers Split #5948

merged 7 commits into from
Nov 2, 2017

Conversation

pasu
Copy link
Contributor

@pasu pasu commented Oct 31, 2017

optimizer this example
the detail is here:#5781

@cesium-concierge
Copy link

Signed CLA is on file.

@pasu, thanks for the pull request! Maintainers, we have a signed CLA from @pasu, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Oct 31, 2017

Thanks @pasu!

@ggetz can you review?

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks for this update @pasu!

For reference, the style/formatting suggestions I made are based on our Coding Guide.

Make sure to update CHANGES.md as well.

@@ -68,27 +68,41 @@
var slider = document.getElementById('slider');
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really relevant to your changes, but can you add infoBox: false as an option when creating the viewer? It will keep the infobox from popping up without any information.


document.getElementById('slider').addEventListener('mousedown', mouseDown, false);
window.addEventListener('mouseup', mouseUp, false);
var bMoveActive = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please call this just moveActive, (or maybe slideActive).

function mouseUp() {
window.removeEventListener('mousemove', sliderMove, true);
}
function move(movement){
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting should be consistant with the rest of the code base.
function move(movement){ -> function move(movement) {
if(!bMoveActive){ -> if (!bMoveActive) {

var slider = document.getElementById('slider');
dragStartX = e.clientX - slider.offsetLeft;
window.addEventListener('mousemove', sliderMove, true);
var nMove = movement.endPosition.x ;//- movement.startPosition.x;
Copy link
Contributor

Choose a reason for hiding this comment

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

nMove -> movementAmount or something more descriptive

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the extra comment

window.addEventListener('mousemove', sliderMove, true);
var nMove = movement.endPosition.x ;//- movement.startPosition.x;
var splitPosition = (slider.offsetLeft + nMove) / slider.parentElement.offsetWidth;
slider.style.left = 100.0 * splitPosition + "%";
Copy link
Contributor

Choose a reason for hiding this comment

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

we use single quotes ' throughout our js

slider.style.left = 100.0 * splitPosition + "%";
viewer.scene.imagerySplitPosition = splitPosition;
}
handler.setInputAction(function(movement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to include the movement parameter in the function if you're not going to use it.

bMoveActive = true;
}, Cesium.ScreenSpaceEventType.PINCH_START);

handler.setInputAction(function(movement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be shoehorned to

handler.setInputAction(move, Cesium.ScreenSpaceEventType.MOUSE_MOVE);
handler.setInputAction(move, Cesium.ScreenSpaceEventType.PINCH_MOVE);

pasu added 3 commits November 1, 2017 10:00
2 variety name bMoveActive -> moveActive
3 function move(movement){ -> function move(movement) {
if(!bMoveActive){ -> if (!bMoveActive) {
4 variety name nMove -> relativeOffset
5 remove the extra comment
6 use single quotes
7 modify handler.setInputAction
@pasu
Copy link
Contributor Author

pasu commented Nov 1, 2017

Hi, @ggetz , thanks for your kind review, I did it.
But it seems there is a small issue about the CHANGES.md

@ggetz
Copy link
Contributor

ggetz commented Nov 1, 2017

Hey @pasu, thanks for the updates! Code looks good to me now.

For CHANGES.md, you should be able to fetch cesium/master and merge it into your branch. You can then resolve any conflicts. However, you'll want to put your changes in a new release ( ### 1.40 - 2017-12-01) since the 1.39 Cesium release is today, and I don't think we'll merge today.

@pasu
Copy link
Contributor Author

pasu commented Nov 2, 2017

ok, I fix the conflicts.

@ggetz
Copy link
Contributor

ggetz commented Nov 2, 2017

Thanks @pasu! @hpinkos This looks good to me!

@hpinkos
Copy link
Contributor

hpinkos commented Nov 2, 2017

Thanks @pasu!

@hpinkos hpinkos merged commit 42324ab into CesiumGS:master Nov 2, 2017
### 1.39 - 2017-11-01

* Added support for right-to-left language detection in labels, currently Hebrew and Arabic are supported. To enable it, set `Cesium.Label.enableRightToLeftDetection = true` at the beginning of your application. [#5771](https://github.com/AnalyticalGraphicsInc/cesium/pull/5771)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ggetz are you sure this CHANGES.md diff is OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pjcozzi I'm not sure what went wrong with this diff, but I just checked and CHANGES is correct in master

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks!

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.

5 participants