Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
PG Lewis
@pglewis
that's not even in CMB2 in my branch
that's in CMB2_Field_Group now
if the merge put that in CMB2 it's going to override the one I'm using
Justin Sternberg
@jtsternberg
yah, when I merged, I remove those changes completely
PG Lewis
@pglewis
and breakage ensues
Justin Sternberg
@jtsternberg
don't believe me? check out my branch compared to yours. https://github.com/WebDevStudios/CMB2/tree/feature/nested-fields-trunk
PG Lewis
@pglewis
to cut down on variable here, though... since I'm seeing the group fields displaying and you're not
Justin Sternberg
@jtsternberg
sure, will use your branch
PG Lewis
@pglewis
I would like you to test the code that's in our branch just to make sure
btw, saving for fields that aren't in a group fields should work fine
like in the user mb or options mb from the snippet lib
that's my enhanced version of the example functions, plus options
PG Lewis
@pglewis
the special "about page" metabox shows, pretty much everything appears to be functioning
there are other things I need to do once it's passing tests and save/load and remaining refactor details are worked out
since we're instantiating fields immediately now, I need to create a test metabox with 250 fields
I need to know any performance impact when abused
Justin Sternberg
@jtsternberg
sorry, caught up in work stuff again
PG Lewis
@pglewis
np, I type fast and can be a nasty chat-bomber :o
Justin Sternberg
@jtsternberg
it's no problem, as long as you're cool w/ me floating in/out of here
PG Lewis
@pglewis
I'm pretty much around until I crash tonight, you're getting concentrated-PG this week
it's important to me to evaluate the full viability of this because it needs to move forward in some fashion before Pods 3 can move forward
and also, if I talk you into this much, my next target is a Fields refactor I'm probably going to have a tough time getting through congress :)
I would like to rid it of the $args array like CMB2 ditched meta_box
Justin Sternberg
@jtsternberg
oof
PG Lewis
@pglewis
again, they're all field properties and the extra layer to get to them doesn't make much sense to me, but moreover, it's becoming the bottle-neck I'm hitting at the final bit of clean up vs tests
but one punch to the gut at a time, I'll let you see what you think of where we've landed thus far
Justin Sternberg
@jtsternberg
ha ha fair enough
PG Lewis
@pglewis
the bottom line is, despite the litter of the getters/setters, I think the API does everything it needs to that way while insulating the details completely now
assuming we deprecated prop() and __get and hopped in a time-machine to when those got removed completely, the CMB2 class is getting fairly tight in that regard, I think
most of the "looks a little messy" stuff left is for back compat I think
I came from a VB6 world in another life
so, when I look at a class like CMB2, I don't think "this class encapsulates code and logic for working with my data". I think "This class encapsulates my data".
thus a good class often ends up being mostly getters and setters in that pattern
PG Lewis
@pglewis
all it's concerned about is managing the data and providing the public interface for the world to do so
in that regard, all the getters and setters aren't duplication like instincts suggest
each property is it's own entity and may need special handling
quite likely each will have different return types
just to kind of explain the philosophy behind the approach a little better
the code here has been very good, but the data structure is where the biggest trouble showed up for nested field support, so I go with what I know for managing data structure
PG Lewis
@pglewis
the intent is that the getters and setters are the majority of your exposed API
PG Lewis
@pglewis
and from an aesthetic standpoint, I think $cmb->set_custom_property( $key, $value ) reads better to someone unfamiliar with some code than $cmb->prop( $key, $value); which doesn't tell you necessarily that a custom prop is being added
the explicitness, while being cumbersome to initially create, creates a readability I've found helpful for maintenance
PG Lewis
@pglewis
I need no run-time context to know what that setter is doing
PG Lewis
@pglewis
we were discussing add_group_field() earlier... that's actually one of the methods that simplifies down nicely due to the API changes:
because you can just call add_field on the parent object, and you can always get the parent object, it boils down to very very simple logic
PG Lewis
@pglewis
backing up a few steps, when you do get the chance I'd like to verify you have our branch working for display the same way I do, when we left off we hadn't achieved that yet
Justin Sternberg
@jtsternberg
Just tested, and is working as expected
as in, it displays the group, but not the group's values