-
Notifications
You must be signed in to change notification settings - Fork 108
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
Implement requireJS and requireCSS #120
Conversation
DarkSide666
commented
Apr 6, 2017
- implements requireJS and requireCSS methods for App
- adds documentation of new metods
- App now have new property $defaultTemplate = 'html.html' for better configurability
src/App.php
Outdated
*/ | ||
public function requireJS($url) | ||
{ | ||
$this->html->template->appendHTML('HEAD', '<script src="'.$this->url($url, null).'"></script>'); |
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.
this should't call url, should simply use the $url , for simplicity.
src/App.php
Outdated
@@ -282,7 +291,7 @@ public function url($args = []) | |||
$page = $args[0]; | |||
unset($args[0]); | |||
|
|||
$url = $page ? ($page.'.php') : ''; | |||
$url = $page ? ($page.($extension ? '.'.$extension : '')) : ''; |
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.
i want to encourage integrators (people who integrate Agile UI into a framework) to rewrite url() method with the goal to properly create links between PAGES. Passing extension to a page is not right.
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.
I think we should rename url() into page() in the future. The full name could be getPageURL().
src/App.php
Outdated
@@ -157,7 +165,7 @@ public function initLayout($layout, $options = []) | |||
} | |||
$layout->app = $this; | |||
|
|||
$this->html = new View(['defaultTemplate'=>'html.html']); | |||
$this->html = new View(['defaultTemplate' => $this->defaultTemplate]); |
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.
application doesn't really have a template, as it's not extending View. Having defaultTemplate would be confusing here. If possible leave it 'html.html' for now as I don't want people to tamper with this too much at this point.
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.
at best we can make it possible for someone to inject "html" property, so
if (!$this->html) {
...
}
docs/app.rst
Outdated
|
||
.. php:method:: requireCSS($url) | ||
|
||
Method to include additional CSS stylesheet in page. |
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.
example would be nice here. Advise people to use CDN.
* allow to pass custom $app->html instead
all change requests addressed |
src/App.php
Outdated
@@ -311,7 +305,7 @@ public function url($args = [], $extension = 'php') | |||
*/ | |||
public function requireJS($url) | |||
{ | |||
$this->html->template->appendHTML('HEAD', '<script src="'.$this->url($url, null).'"></script>'); | |||
$this->html->template->appendHTML('HEAD', '<script src="'.$url.'"></script>'); |
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.
wonder if we can use getTag here...
implements #50 |