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
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
PG Lewis
@pglewis
right, that's as expected
Justin Sternberg
@jtsternberg
so we're on the same page
PG Lewis
@pglewis
it's safe to assume the remaining pieces /could/ be wired up
non grouped fields load save as normal, I only broke saving on the group fields when I swicthed them over to support nesting
I think the next step is decide if you want to adopt this approach and come up with a roadmap for working it into trunk
it's become far enough removed that merges aren't likely to work 1 to 1 and maintaining two branches is no fun
assuming we can use trunk as a beta testing branch, so we can also hit hit hard with code in the wild, that could be an option
I think a couple more days' work and it could be in shape enough to hit with further testing if we continued on
Justin Sternberg
@jtsternberg
I'd say once saving works for grouped fields, we can look more seriously at merging to trunk
PG Lewis
@pglewis
yeah, that's a bare minimum