Closed (fixed)
Project:
Shariff Social Media Buttons
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
15 Jan 2015 at 16:49 UTC
Updated:
13 Feb 2015 at 14:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
crizThanks! The field works as expected and there it seems as there are no coding style issues.
However, I have the following suggestions:
- Let's use the theme_shariff function to display the buttons, to be consistent.
- For this we need to prepare the preprocess variables as done for the block. Maybe we should introduce a helper function for this, so we don't have this code multiple times?
- I don't think it's necessary to add the data-url? However, having the url in the settings form doesn't make really sense either...
Comment #2
sboehlke commentedHey, I'll agree with most of your points: to use the theme function for the fields makes sense, and a helper function to prepare the data settings would also be useful. But about the data-url: you're right about that we don't need to set the data-url in the settings form, because the URL you want to share shouldn't be static. But in my opinion you need to set the data-url when you generate the field output for a node, because the URL of the page the node is displayed on is not necessarily the URL of the node itself (e.g. when you have several nodes displayed on the same page with their respective sharing buttons, you probably want to share the node itself when you click one of these buttons, and not the "parent-page", where all nodes are displayed). I'll write another patch this week to provide a helper function and to include the theme function in the field output, if there aren't any objections from your side.
Comment #3
crizOkay, I see. To display the button on teaser-lists setting the data-url makes absolutely sense. :)
Comment #4
sboehlke commentedHere's the new Patch. It includes everything from the first patch and the following changes:
* Update function to remove the shariff_url variable
* Removed the shariff_url input from the admin form
* Removed the validation of shariff_url in the admin form
* Helper function _shariff_get_settings() to get the settings
* Admin form settings default values will be set with the help of _shariff_get_settings()
* Moved the "safety checks" of the settings (check_url, urlencode ect.) from shariff_block_view to shariff_preprocess_shariff(...)
* Minor changes to clean up the code (added some blank lines to the admin form, change the order of the variables in the theme function to fit it to the order of the admin form)
Comment #5
crizLooks good! Will test it accurately in the next days.
Comment #7
crizCommitted. My changes:
- Fixed panels plugin to work with new settings helper function.
- English default language instead of German.
- Used entity_uri() instead of only url() to make this also work for entities other than nodes.
- Changed readme to inform about the field.