Closed (fixed)
Project:
Contact Info
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
27 Aug 2013 at 11:22 UTC
Updated:
22 Aug 2015 at 18:20 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #0.0
amonteroLink module
Comment #1
dboulet commentedThe code looks ok, but I’m going to mark this as won’t fix. If we add tokens for lat/long, then we’ll have to add tokens for all the fields—I don’t think that the need for this feature would be common enough to warrant this additional code. My goal with this module was always to keep it as simple as possible.
Nothing is stopping you from using this code in a separate module, you could easily create a contactinfo_tokens module to add this functionality.
Thanks for the suggestion.
Comment #2
amonteroAs adding code in the main module file and/or keeping it lean and tidy is desirable for you, I can reroll the patch but placing the token functionality in a sepatare contactinfo.token.inc file to take advantage of token module implementing hook_hook_info:
https://api.drupal.org/api/drupal/modules!system!system.module/function/...
This way you'll keep the token functionality out of sight. Also, it will be loaded only when needed, good for performance.
The proposed functionality would just enhance the module without any end-user noticing, but would allow more site devs to do fancy things with it (multisites and distributions come to mind).
I agree on the need to provide the remaing contact fields, too. But creating a new module just for this simple functionality looks overkill to me. Just adding now and placing a "initial token support" banner linking here in the module page would make the module sexier to site devs ;)
Comment #3
dboulet commentedThanks for the additional info, adding this code to an include file makes sense.
I worked on a new patch which adds all of the tokens, please give it a review to make sure that it meets your needs.
Comment #4
amonteroJust two small changes:
* As per UI guidelines, using ' instead of ’ for "site's".
* Changed printing-style double quotes (“Personal”) to regular, since in my non-English browser, they displayed weird chars. Also, as per UI guidelines, regular double quotes are used everywhere.
Here are the patch interdiffs:
Other than those two, the attached patch is the same which works great in my tests. Thank you very much.
Comment #5
dboulet commentedCan you point me to the quideline that recommends using straight quotes? All I could find was the following, which actually recommends doing the opposite:
https://drupal.org/style-guide/content#punctuation
Smart quotes are generally considered correct, typographically—if they are not displaying properly, then you might need to change the encoding of your page.
More info:
http://smartquotesforsmartpeople.com/
Comment #6
amonteroOps. I was basing #2 in an assumption based on my above referenced and linked page alone. I recall seeing there to use " quotes somewhere. Checked your provided references and put back smart quotes, but using HTML entities this time. They display OK in my browser, now.
Also, bookmarked the nice http://smartquotesforsmartpeople.com/ . Thanks for the link. Since I'm under Ubuntu, here's how to pull smart quotes on Ubuntu, for completeness.
Patches interdiff:
Comment #7
dboulet commentedDo the quotes only render properly for you if you insert them as HTML entities? You shouldn’t have a problem rendering them if you use UTF8 everywhere. Same goes for proper apostrophes.
I’d rather use proper curly quotes and apostrophes, they make the text much easier to read than having to use entities and escaping straight single quotes.
Comment #8
dboulet commentedThanks again for your help amontero, I just pushed up the changes to add token support to the module.
In the end, I decided to use straight quotes like you suggested, as this is the style used in core modules. I still think that proper quotes are the way to go, but that can be left for the future. Interestingly, I found this old issue suggesting updates to the quote style in core—it wasn’t received very warmly:
#89499: Replace " signs with “curly quotes”
Comment #10
amonteroGreat to see it added to the module, thanks.
Too bad about the quote issue, now that I was already sold on it.
Minor issue: add this to the project description feature list.
Comment #11
dboulet commented