Linking to office maps

VERIFIED FIXED in Future

Status

VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: andy+bugzilla, Assigned: caseybecking)

Tracking

unspecified
Future

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: r=94529 b=trunk)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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 :)
Totally makes sense. Marking for future.

Or maybe you want to submit a patch? :)
Target Milestone: --- → Future
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.
Assignee: nobody → caseybecking

Comment 3

7 years ago
Casey, welcome aboard!
(Assignee)

Comment 4

7 years ago
Created attachment 555893 [details] [diff] [review]
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.
Attachment #555893 - Flags: review?(anthony)
Created attachment 556032 [details] [diff] [review]
Proper diff

Putting the patch into a diff.
Attachment #555893 - Attachment is obsolete: true
Attachment #555893 - Flags: review?(anthony)
Attachment #556032 - Flags: review?
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.
Attachment #556032 - Flags: review? → review-
(Assignee)

Comment 7

7 years ago
Awesome i will fix those issue and re-post the diff.
(Assignee)

Comment 8

7 years ago
Created attachment 556156 [details] [diff] [review]
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.
Attachment #556156 - Flags: review?(anthony)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 681862

Comment 10

7 years ago
Apologies, meant to do this on another bug.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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.
Attachment #556156 - Flags: review?(anthony) → review-
(Assignee)

Comment 12

7 years ago
Created attachment 556704 [details] [diff] [review]
adding hash tag to url

Also is there a proper naming convention for the files when i submit a diff?
Attachment #556704 - Flags: review?(anthony)
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.
Attachment #556704 - Flags: review?(anthony) → review-
(Assignee)

Comment 14

7 years ago
Created attachment 557019 [details] [diff] [review]
Mozilla map diff v4

This one work better and cleaner. Thank you
Attachment #557019 - Flags: review?(anthony)

Updated

7 years ago
Attachment #556032 - Attachment is obsolete: true

Updated

7 years ago
Attachment #556156 - Attachment is obsolete: true

Updated

7 years ago
Attachment #556704 - Attachment is obsolete: true
Comment on attachment 557019 [details] [diff] [review]
Mozilla map diff v4

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

Yeah, thanks !
Attachment #557019 - Flags: review?(anthony) → review+
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.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Keywords: qawanted
Resolution: --- → FIXED
Whiteboard: r=94529 b=trunk
(Reporter)

Comment 17

7 years ago
Thanks
(Assignee)

Comment 18

7 years ago
Awesome thank you, i see what you did. Any others i can work on?
Pushed r94586 in prod.

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

Comment 21

7 years ago
Thank you. Wonder if it gets more view than my day to day job quiksilver.com ?
verified fixed
Status: RESOLVED → VERIFIED
Component: www.mozilla.org/firefox → www.mozilla.org
Product: Websites → Websites
Component: www.mozilla.org → General
Product: Websites → www.mozilla.org
You need to log in before you can comment on or make changes to this bug.