Skip to content

Conversation

Scrum
Copy link
Member

@Scrum Scrum commented Nov 22, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6abbc6d on v0.3.0 into 00b41b0 on master.

@Scrum
Copy link
Member Author

Scrum commented Nov 22, 2017

@voischev @mrmlnc approve ?

Copy link
Member

@mrmlnc mrmlnc left a comment

Choose a reason for hiding this comment

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

Looks good and very simple 🌮

index.js Outdated
var directives = options.directives || defaultDirectives;

for (var i = 0; i < directives.length; i++) {
if (name.toLowerCase() === directives[i].name) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it will be better?

var directive = directives[i];

results.push(directive.start + data + directive.end);

expect(parser('<!doctype html>')).to.eql(['<!doctype html>']);
});

it('should be parse directive', function() {
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, need some test case when <?php ...> nested into any html tag (for example, <div>).

Copy link
Member Author

Choose a reason for hiding this comment

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

Very thanks, i find bug, and fixed him. )

Choose a reason for hiding this comment

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

<?php wp_nav_menu(array('theme_location' => 'primary', 'menu_class' => 'header-nav-bar')); ?>
is not working correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

@IgorMotorny Hi, I need more information. Create a separate Issue and describe the problem case. Thanks.

@voischev voischev self-requested a review November 22, 2017 09:50
Copy link
Member

@voischev voischev left a comment

Choose a reason for hiding this comment

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

Cool 👏

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5e1650a on v0.3.0 into 00b41b0 on master.

@Scrum
Copy link
Member Author

Scrum commented Nov 22, 2017

@voischev @mrmlnc, pleas get me access on npmsj for publish this minor.

@Scrum
Copy link
Member Author

Scrum commented Nov 22, 2017

@posthtml/collaborators LGTM ?

@Scrum Scrum merged commit ee1f515 into master Nov 22, 2017
@Scrum Scrum deleted the v0.3.0 branch November 22, 2017 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants