Skip to content

Conversation

@janis91
Copy link

@janis91 janis91 commented Aug 29, 2016

As discussed in #889 , I added the capability for passing required scripts and styles to the template response. The scripts and styles will be added to the template after that.

An example constructor call (scripts work the same):

$styles = array(
            'vendor' => array('select2/select2'),
            'app' => array('nextnotes' => array('font-awesome.min',
                'simplemde.min',
                'markdown.min',
                'style')
            )
        );
new TemplateResponse('nextnotes', 'main', array(), 'user', $styles);

I will provide the tests, if this is what you were expecting @rullzer.

Added the capability of passing required scripts and styles to the template response.
@mention-bot
Copy link

@janis91, thanks for your PR! By analyzing the annotation information on this pull request, we identified @MorrisJobke, @BernhardPosselt and @DeepDiver1975 to be potential reviewers

@BernhardPosselt
Copy link
Member

What would scripts look like?

@janis91
Copy link
Author

janis91 commented Aug 29, 2016

full example:

$styles = [
    'vendor' => ['select2/select2'],
    'app' => [
        'nextnotes' => [
            'font-awesome.min',
            'simplemde.min',
            'markdown.min',
            'style',
        ],
    ],
];
$scripts = [
    'vendor' => ['select2/select2'],
    'app' => [
        'nextnotes' => [
            'notes',
            'tags',
            'view',
            'app',
        ],
    ],
];
new TemplateResponse('nextnotes', 'main', [], 'user', $styles, $scripts);

@BernhardPosselt
Copy link
Member

Hm I'd say the idea is good but that's one giant constructor. Think of inlining everything ;D

What about combining both arrays in another array, so instead of $scripts, $styles, $htmlImports, ... we would simply pass something like $assets where $assets is:

[
    'scripts' => ...
    'styles' => ...
...

@rullzer
Copy link
Member

rullzer commented Aug 30, 2016

Thanks for the PR @janis91

seeing it now. I'm not a big fan of magic arrays to pass to constructors.
I'm mayb leaning more to a few functions on the template response:

addVendorScript, addScript etc.

That might make it more explicit and a bit more abstract?
Mmmm need to do some more thinking about that.

@janis91
Copy link
Author

janis91 commented Aug 30, 2016

I could remove it from the constructor and add functions for every case. But recently I recognized the style and script functions in /lib/private/legacy/template/functions.php. Maybe we have to discuss if the approach proposed here is really a step forward?

added as functions.
@raghunayyar
Copy link
Member

Hi, can you add labels to this PR to have a better clarity if it is a WIP or a complete PR?
Thanks.

@rullzer rullzer added enhancement 2. developing Work in progress labels Sep 1, 2016
@rullzer rullzer added this to the Nextcloud 11.0 milestone Sep 1, 2016
@MorrisJobke
Copy link
Member

@rullzer Anything new?

* @param string $renderAs how the page should be rendered, defaults to user
* @since 6.0.0 - parameters $params and $renderAs were added in 7.0.0
* @param array $styles an array of styles which sould be added to the template
* @param array $scripts an array of scripts which should be added to the template
Copy link
Member

Choose a reason for hiding this comment

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

This then needs to be removed.

@rullzer
Copy link
Member

rullzer commented Oct 19, 2016

Mmmm I'm still not really sure here.
@janis91 maybe we should indeed for now stick with the stuff that is there. And revisit this when we get a decent template engine. Does that sound good?

@janis91
Copy link
Author

janis91 commented Oct 19, 2016

Yeah I am with you!

@BernhardPosselt
Copy link
Member

FYI I found this idea to be a quite interesting take on templates https://r.je/transphporm-php-templates.html

Not saying that we should adopt it but its interesting

@rullzer
Copy link
Member

rullzer commented Oct 19, 2016

Ok lets close this then.

Thnaks for the link @BernhardPosselt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants