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

allow extention of setProperty(), add socialmedia option #136

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gegere
Copy link

@gegere gegere commented Dec 18, 2018

Hello, great job continuing to enhance the vCard repo allowing so much reach with a simple file.

After using a simple vCard script for 5 years it is time for an upgrade after reviewing additional properties and functions I found $this repo. Here are 2 suggestions:

  1. Adding a Social Media option, this will allow twitter, flickr, linkedin, etc... to be listed in the vCard

  2. Change the property type for setProperty from private to protected which would allow the ability to extend the class with custom functionality while maintaining the ability to use PHP Composer.

There are a few updates I'd list to submit if this request it approved. Several "type field" options which could be improved, more on this later. Thanks!

Here is a working example with modifications:
vCard for Jason Gegere

src/VCard.php Outdated Show resolved Hide resolved
* @param string $addr The address of the social media platform.
* @return $this
*/
public function addSocial($type = '', $url)

Choose a reason for hiding this comment

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

Please add tests for this function

Copy link
Author

Choose a reason for hiding this comment

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

On my todo list.

@@ -245,6 +245,13 @@ protected function parse()
$key = !empty($types) ? implode(';', $types) : 'default';
$cardData->url[$key][] = $value;
break;
case 'SOCIALMEDIA':

Choose a reason for hiding this comment

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

Please add tests for parsing this data

Copy link
Author

Choose a reason for hiding this comment

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

On my todo list, do you have any examples?

@melroy89
Copy link

melroy89 commented Dec 27, 2018

The vCard standard says we need to use the URL for social media references.This will make it incompatible with the standard.

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.

3 participants