Skip to content
This repository was archived by the owner on Nov 21, 2019. It is now read-only.

Use Twig macros rather than including templates#76

Merged
thewilkybarkid merged 11 commits intolibero:masterfrom
thewilkybarkid:macros
Apr 10, 2019
Merged

Use Twig macros rather than including templates#76
thewilkybarkid merged 11 commits intolibero:masterfrom
thewilkybarkid:macros

Conversation

@thewilkybarkid
Copy link
Copy Markdown
Contributor

@thewilkybarkid thewilkybarkid commented Apr 2, 2019

Replaces the _attributes.twig and the badly-named _text.twig templates with macros.

(Might be some other things where macros make sense, to be investigated.)

@thewilkybarkid
Copy link
Copy Markdown
Contributor Author

Seems to noticeably help Browser.

Before:

Screenshot 2019-04-02 at 11 39 51

After:

Screenshot 2019-04-02 at 11 39 56

{%- set attributes = attributes|default({})|merge({class: attributes.class|default([])|merge(['paragraph']) }) -%}

</p>
{{- html.element('p', text, attributes) -}}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure this is better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Going to revert this. Bit too much magic.

{%- import 'atoms-html' as html -%}

{%- include 'atoms-text' with {nodes: text} -%}
{%- set attributes = attributes|default({})|merge({class: attributes.class|default([])|merge(['paragraph']) }) -%}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Been going round the houses trying to find a way to improve this. Can't, and keep coming back to #40...

@thewilkybarkid thewilkybarkid marked this pull request as ready for review April 3, 2019 13:46
@thewilkybarkid thewilkybarkid requested a review from a team as a code owner April 3, 2019 13:46
Copy link
Copy Markdown
Contributor

@davidcmoulton davidcmoulton left a comment

Choose a reason for hiding this comment

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

Not au fait with the details of this, but looks okay form a cursory look.

@thewilkybarkid thewilkybarkid merged commit d61a93e into libero:master Apr 10, 2019
@thewilkybarkid thewilkybarkid deleted the macros branch April 10, 2019 13:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants