681160 - Linking to office maps

Status: VERIFIED FIXED

(www.mozilla.org :: General, defect)

Description

[Andy McKay]

I organize a few events at the mozilla Vancouver office.  It would be nice if I could link directly to a map of the Vancouver office. Something like:

http://www.mozilla.com/en-US/about/contact.html#vancouver

And it would show the vancouver office. Instead of having to say: "go to http://www.mozilla.com/en-US/about/contact.html and click on Vancouver".

Repeat for other offices :)

Comment 1

[Anthony Ricaud (:rik)]

Totally makes sense. Marking for future.

Or maybe you want to submit a patch? :)

Comment 2

[Anthony Ricaud (:rik)]

Assigning to Casey, a contributor that wants to work on open source code.

Casey: Once you get something working, please submit a patch and ask me for a review.

Comment 3

[John Slater]

Casey, welcome aboard!

Comment 4

[Casey Becking]

Attachment: Adding the use of hash tag to the url for map navigation

Not sure how we would let the user know what to put in the url though. We may want to do the linking for the map differently.

Comment 5

[Anthony Ricaud (:rik)]

Attachment: Proper diff

Putting the patch into a diff.

Comment 6

[Anthony Ricaud (:rik)]

Comment on attachment 556032 [details] [diff] [review]
Proper diff

Review of attachment 556032 [details] [diff] [review]:
-----------------------------------------------------------------

Casey: Thanks for submitting a first pass so quickly.

Next time, please submit a real diff. It's easier to read and easier to comment part of the code.

> Not sure how we would let the user know what to put in the url though. We
> may want to do the linking for the map differently.
You could set a different hash inside the goToLocation method.

::: js/mozilla-map.js
@@ +65,3 @@
>  }
> +//This will check for a hash tag if there is a hash tag it will send the map to that location
> +Mozilla.Map.prototype.getLocationHash = function()

Don't name a function that updates the page with "get".

@@ +65,5 @@
>  }
> +//This will check for a hash tag if there is a hash tag it will send the map to that location
> +Mozilla.Map.prototype.getLocationHash = function()
> +{
> +	var hash = window.location.href.slice(window.location.href.indexOf('#') + 1);

You can use window.location.hash. It's just the hash part, including the #.

@@ +69,5 @@
> +	var hash = window.location.href.slice(window.location.href.indexOf('#') + 1);
> +	if(hash !== undefined){
> +		this.goToLocation(hash);
> +		//not sure how to get around the normal hash tag behavior
> +		window.scroll(0,0);

Actually, don't get around it. Use another hash that don't collide with id-pages. Maybe prefix with "map. So it will be #map-new-zealand.

Comment 7

[Casey Becking]

Awesome i will fix those issue and re-post the diff.

Comment 8

[Casey Becking]

Attachment: Mozilla map diff v2

Lets try this again, hopefully this is the correct diff your looking for. Sorry for the back and fourth once i get the workflow figured out it shouldnt take so long.

Comment 10

[David Boswell]

Apologies, meant to do this on another bug.

Comment 11

[Anthony Ricaud (:rik)]

Comment on attachment 556156 [details] [diff] [review]
Mozilla map diff v2

Review of attachment 556156 [details] [diff] [review]:
-----------------------------------------------------------------

> Lets try this again, hopefully this is the correct diff your looking for.
> Sorry for the back and fourth once i get the workflow figured out it
> shouldnt take so long.
Two patches on two days is amazing :) Don't worry, the back and fourth is the normal process for a review.

::: js/mozilla-map.js
@@ +37,2 @@
>  	this.goToLocation('mountain_view');
> +	this.updateLocationHash();

I think the best logic here is: 
1) Check the hash
2) If no valid hash provided, go to a default location (Mountain View)
3) Update the hash for copy/paste pleasure.
You can put this logic inside updateLocationHash.

@@ +65,4 @@
>  }
> +//This will check for a hash tag if there is a hash tag it will send the map to that location
> +// hash names that can be used are #map-mountain_view, #map-new-zealand, #map-china,
> +// #map-europe-paris, #map-us-san-francisco, #map-japan, #map-canada-toronto, #map-canada-vancouver

Just give an example. By providing the full list, it will be out of sync when the next office will be added.

@@ +70,5 @@
> +{
> +	var hash = window.location.hash;
> +	var hashName;
> +	var cleanHashName;
> +	if(hash !== undefined){

hash is never undefined. You want to test against an empty string here I guess.

@@ +71,5 @@
> +	var hash = window.location.hash;
> +	var hashName;
> +	var cleanHashName;
> +	if(hash !== undefined){
> +		for (var i in this.locations) {

Use location instead of i here, easier to read.

@@ +73,5 @@
> +	var cleanHashName;
> +	if(hash !== undefined){
> +		for (var i in this.locations) {
> +		hashName = '#map-' + i;
> +		console.log(i);

Please remove debug statements for production and indent appropriately.

@@ +77,5 @@
> +		console.log(i);
> +			if(hashName === hash){
> +				//need to remove the map- part and goto the hashName
> +				cleanHashName = hashName.replace("#map-", "");
> +				this.goToLocation(cleanHashName);	

You can remove those 3 lines and replace by goToLocation(location) here.

Comment 12

[Casey Becking]

Attachment: adding hash tag to url

Also is there a proper naming convention for the files when i submit a diff?

Comment 13

[Anthony Ricaud (:rik)]

Comment on attachment 556704 [details] [diff] [review]
adding hash tag to url

Review of attachment 556704 [details] [diff] [review]:
-----------------------------------------------------------------

This works great! Just need a few changes for a last round.

Don't worry about the filename for the diff, I can't see it :)

::: js/mozilla-map.js
@@ +37,5 @@
> +	if (window.location.hash !== "") {
> +			this.updateLocationHash();
> +	}else{
> +			this.goToLocation('mountain_view');	
> +	}

Can you move this over to updateLocationHash with a safer default?

Exit the function when a hash matches and go to the default otherwise.

@@ +69,5 @@
> +Mozilla.Map.prototype.updateLocationHash = function()
> +{
> +	var hash = window.location.hash;
> +	var hashName;
> +	var cleanHashName;

Remove this since it is not used.

@@ +105,4 @@
>  		var that = this;
>  		$(title_anchor).click(function (e) {
>  			e.preventDefault(); that.goToLocation(id);
> +			window.location.hash = '#map-' + id;

Move this inside goToLocation so that all the Update Map links also change the hash.

Comment 14

[Casey Becking]

Attachment: Mozilla map diff v4

This one work better and cleaner. Thank you

Comment 15

[Anthony Ricaud (:rik)]

Comment on attachment 557019 [details] [diff] [review]
Mozilla map diff v4

Review of attachment 557019 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, thanks !

Comment 16

[Anthony Ricaud (:rik)]

I just pushed this to trunk with r94529. See in action on http://www-trunk.stage.mozilla.com/en-US/about/contact.html.

Casey: Thanks for this! I just tweaked a bit the indentation and the default location part.

Comment 17

[Andy McKay]

Thanks

Comment 18

[Casey Becking]

Awesome thank you, i see what you did. Any others i can work on?

Comment 19

[raymond [:retornam] (inactive)]

qa-verified-trunk http://www-trunk.stage.mozilla.com/en-US/about/contact.html#map-canada-vancouver

Comment 20

[Anthony Ricaud (:rik)]

Pushed r94586 in prod.

Casey: Your first patch is now visible to our millions of visitors ;)

Comment 21

[Casey Becking]

Thank you. Wonder if it gets more view than my day to day job quiksilver.com ?

Comment 22

[raymond [:retornam] (inactive)]

verified fixed