Skip to content
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

Support v-for="(item, index)" #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

lucaswerkmeister
Copy link
Member

This adds rudimentary support for iterating over a value with the optional second argument for the index. (Because lists and objects are so similar in PHP, this also doubles as support for iterating over an object with the optional second argument for the key; the optional third argument for the index in that case is not supported, though.)

Note that, due to the limited JS evaluation in this templating engine, there’s not a whole lot you can do with the index. In the WikibaseLexeme use case, the index is only actually used in a part of the template that’s never server-side rendered; the loop still has to be declared using the index form even for the PHP renderer, though.

The unrelated change from v-else="" to v-else in the fixtures was generated by npm run-script populate-fixtures, presumably due to a newer Vue.js version now being used – we don’t commit the package-lock.json (yet?). I figured I might as well add it, since one of the fixtures is changed anyways to test the feature.


The corresponding WikibaseLexeme change is I9b41958eec.

@lucaswerkmeister
Copy link
Member Author

CI fails on PHP 5.5 and HHVM; I’m proposing to kick those out of .travis.yml in #17.

@addshore
Copy link
Contributor

Needs a rebase apparently D:

This adds rudimentary support for iterating over a value with the
optional second argument for the index. (Because lists and objects are
so similar in PHP, this also doubles as support for iterating over an
object with the optional second argument for the key; the optional third
argument for the index in that case is not supported, though.)

Note that, due to the limited JS evaluation in this templating engine,
there’s not a whole lot you can do with the index. In the WikibaseLexeme
use case, the index is only actually used in a part of the template
that’s never server-side rendered; the loop still has to be declared
using the index form even for the PHP renderer, though.

The unrelated change from v-else="" to v-else in the fixtures was
generated by `npm run-script populate-fixtures`, presumably due to a
newer Vue.js version now being used – we don’t commit the
package-lock.json (yet?). I figured I might as well add it, since one of
the fixtures is changed anyways to test the feature.
@lucaswerkmeister
Copy link
Member Author

Done (conflicted with the change from /** @test */ to public function test…).

@@ -240,10 +240,21 @@ private function handleFor( DOMNode $node, array $data ) {
list( $itemName, $listName ) = explode( ' in ', $node->getAttribute( 'v-for' ) );
$node->removeAttribute( 'v-for' );

foreach ( $data[$listName] as $item ) {
if ( preg_match( '/\((?P<itemName>[^,]+)\s*,\s*(?P<keyName>[^\)]+)\)/', $itemName, $matches ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this uses an extremely crude regex to match variable names; I’m not sure if this library has a better regex (snippet) for that elsewhere, though.

@addshore
Copy link
Contributor

Should I leave this to see how things continue to develop with the gerrit patch? (https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WikibaseLexeme/+/589656/ ) or would you rather this get looked at now? :)

@lucaswerkmeister
Copy link
Member Author

We can leave this alone for now, I think.

@lucaswerkmeister
Copy link
Member Author

This is no longer required for the focus changes. We could still merge it if we want to, or leave it open until someone else needs it…?

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

Successfully merging this pull request may close these issues.

3 participants