Add template logic for nested data #30
Conversation
I.E. Child objects can now have arrays of iterable children.
There was a problem hiding this comment.
@JoshuaCarter, have left a couple of things that jumped out to me, but looks good to merge and tag otherwise. The only thing missing actually would be a bump to the branch-alias in composer.json since this looks like a feature release.
| * | ||
| * And implement '__set_state' to return a constructed ArrayList. | ||
| */ | ||
| class ArrayListExportable |
There was a problem hiding this comment.
Since you're defining a new class here, this should be contained in a new file.
There was a problem hiding this comment.
I did it that way because it's only for internal use in ComponentService, but I can see how it's bad, so I've split it out into it's own file.
| * @param bool $root Ignore this | ||
| * @return string Executable template logic | ||
| */ | ||
| private static function exportNestedDataForTemplates(array $array, $root = true) |
There was a problem hiding this comment.
Is there a particular reason this is static?
There was a problem hiding this comment.
It could be a method, but as a static function it's clear that it doesn't interact with $this at all, it takes nestsed data, recurses it, and spits back the output. Whilst I think this is technically better/clearer, I'm happy to change it if you prefer.
There was a problem hiding this comment.
Hmm, while I don't disagree, if we're going to make it static I would lean towards making it public so it can be accessed from outside the service class. But for consistency purposes, it should probably stay public and non-static. Happy to leave this one to your judgement, just let me know what you decide on :)
There was a problem hiding this comment.
My vote is:
- private: because it's too specific and niche to be externally useful. Else I would be all for making it public.
- static: because it works independant of
$thiswhich is context I would find useful when navigating unfamiliar code, etc.
But I'm the noob here, so happy to be overridden.
There was a problem hiding this comment.
As discussed IRL, I can always see someone trying to access a private function, no matter how niche it is :) But I agree, private and static makes the most sense!
|
The branch alias on 1.1 branch was 1.0.x. I've bumped it to 1.2.x, but perhaps it should only be 1.1.x? |
|
Ahh, forgot this was on the 1.1 branch. That likely means somebody forgot the bump the branch alias previously, so yeah 1.2.x would be correct. However, the branch alias only really matters for the master branch, so we just need to make sure that's the one we bump when it comes back into SS4 :) |
JSON data given to template components can now have nested arrays of iterable children (see the SubNavigation example from front-end-tooling). Test included.
This PR resolves:
I would suggest making a new release for this addition.
Note: This change will also need to go into master, but as the source file is different, that will need to be a separate PR.