-
Notifications
You must be signed in to change notification settings - Fork 6
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
add optional more optional parameters to personalize more the embed #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure the PR has a description.
Also, review the comments in the code.
public $width = '100%'; | ||
public $height = '800'; | ||
public $width; | ||
public $height; | ||
|
||
/** | ||
* Default constructor | ||
* | ||
* @param $url string base url for the Metabase installation | ||
* @param $key int secret Metabase key | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add the new parameters to the documentation (phpdoc block)
src/Embed.php
Outdated
|
||
/** | ||
* Default constructor | ||
* | ||
* @param $url string base url for the Metabase installation | ||
* @param $key int secret Metabase key | ||
*/ | ||
public function __construct($url, $key) | ||
public function __construct($url, $key, $border = true, $title = false, $width = '100%', $height = '800') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider ordering the parameters so the most often used ones are first - perhaps: title, width, height, border
Align comments.
Thank you for your contribution |
Added the ability to change the "default" settings if you want to customize your iframe a bit more.