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

Field Types: Make object oriented, support custom field types. #45

Open
earboxer opened this issue Apr 13, 2019 · 5 comments
Open

Field Types: Make object oriented, support custom field types. #45

earboxer opened this issue Apr 13, 2019 · 5 comments
Labels

Comments

@earboxer
Copy link
Contributor

Currently, field types are defined by simple strings. This makes it easy to define a list of attributes, but lets say I wanted some more custom field types

class User extends OtterResource
{
    public function fields() {
        return [
            'name' => 'text',
            'password' => 'password',
            'email' => 'email',
            'purpose_statement' => 'textarea',
            'biography' => 'textarea',
            'favorite_color' => 'color',
        ];
    }
...

Currently, #43 implements the textarea by changing FormComponent.vue, SingleResourceComponent.vue, and TableComponent.vue. While this works for the previous example, it's hard to customize.

For example, if I wanted the table view of 'favorite_color' to just show the favorite color (instead of the string #000000, I would need to customize FormComponent.vue to have another if statement in it. If I wanted to increase the number of rows in the textarea element for biography, I would need to define a textarea-longer type, and write if statements to make sure those got carried over.

Proposal: Create an Object Interface defining rendering for the listing, viewing, and editing interfaces. Allow parameters to be set for each one. (Also allow these to define defaults for 'hidden' and 'validations')

Possible usage

    public function fieldTypes() {
        return [
            'name' => [BasicField::class, ['type' => 'text'],],
            'password' => PasswordField::class,
            'email' => [BasicField::class, ['type' => 'email'],],
            'purpose_statement' => TextAreaField::class,
            'biography' => [TextAreaField::class, ['rows' => 20],],
            'favorite_color' => ColorField::class,
        ];
@earboxer
Copy link
Contributor Author

earboxer commented Apr 13, 2019

Possible implementation of interface

interface FieldTypeInterface
{
    public function __construct(array $params);
    /**
     * @brief render functions take the object and the key and return something Renderable.
     */
    public function renderEdit($object,$key,$value);
    public function renderList($object,$key,$value);
    public function renderView($object,$key,$value);
    public function validations() : array;
}

Questions to answer

  • Should we be passing the object to this function, or the value from the object?
    • Not passing the object will force developers to write more code in their models
  • How should we treat fields types that use multiple columns from the model?
    • 'lat' => [LocationField::class, ['latitudeKey' => 'lat', 'longitudeKey' => 'lon']] -- and having renderEdit() create fields with name='lat' and name='lon'
    • after defining getLatLonAttribute and setLatLonAttribute in eloquent model, 'lat_lon' => LocationField::class,
    • 'lat,lon' => LocationField::class -- "native"-looking support for multiple fields

@earboxer
Copy link
Contributor Author

earboxer commented Apr 13, 2019

This seems like a breaking change, since hidden() wouldn't really be needed (just have the fieldType return nothing for renderList and renderView).

Changelog for users

  • Deprecate fields() -- write it to automatically use BasicField and PasswordField so most people's existing OtterResources would still work fine.
  • Create fieldTypes() accepts an array of key => classname, key => [classname], key => [classname, [optionA => valueA]]
  • hidden() is deprecated -- does nothing (users are encouraged to pass 'hidden' => TRUE as parameters to field types that use it)
  • validations() now have default values for some field types. They can of course be overwritten (and should be in most cases)

Do you think it would be worth it to provide some kind of support for hidden() so that it continues to work? This would depend on the number of production systems running otter and the number of hidden fields that they use (other than password). If this project had version numbers (using semantic versioning), we would want to bump up the version number.

@zanechua
Copy link
Member

I think we can work with this and target v1.0.0 since I have not actually versioned it to anything at the moment. This package is still in it's infancy so I think breaking changes.. would be a given. I'm not too sure about the usage of Otter in production as I don't have any tracking of sorts but if we base the amount of users off the statistics from packagist, I think we're good to make the breaking change for a better modular package.

@earboxer
Copy link
Contributor Author

earboxer commented Feb 22, 2020

Thinking more about this: passing them as objects shouldn't hurt anything, performance-wise, we're only ELV-ing one thing at a time anyway. Also, then we can just allow each one to define its own constructor! Hooray!

      return [
            'name' => new BasicField('text'),
            'password' => new PasswordField(),
            'email' => BasicField('email'),
            'purpose_statement' => TextAreaField(),
            'biography' => TextAreaField(['rows' => 20]),
            'favorite_color' => ColorField(),

And looking at my FieldTypeInterface from above, I'm not sure why I wanted to pass object, key, and value. Presumably $value is always equal to $object->$key. It would lead to inconsistencies if we did it some other way...

This leaves us still without a way of getting two keys. I think the 'lat,lon' idea is(could be) the best. (if we use the fields to optimize the query, we'd also have to split this there, but I think the getLatLonAttribute way provides the same kind of challenge... maybe optimizing queries isn't a goal of this project).

Personally, I'm not too concerned with validations (I just don't know that I'd use it), so I won't make major suggestions for change here (so, maybe I'll accidentally break it on implementation). Architecturally, it seems like something that should be moved inside renderEdit (at least for the client-side).

@earboxer
Copy link
Contributor Author

A few concerns with allowing or recommending the use of non-fields as keys:

They can't be used in select or where (which we may want to improve the search box in the future (FELV-ing*?)) or orderBy (we'll want server-side sorting).

* Facet, Edit, List, View

For faceting, I think we would want to allow the search box to modify the model query that is happening with 1 or more where statements.

e.g.
lat > 42.8 could generate ->where('lat','>', 42.8)

Different fields might want to define their own verbs:
lat,lon within 87G7PX00+ could generate ->where('lat', '>', 40.7)->where('lat','<',40.75)->where('lon','>'-74.05')->where('lon','<',40.75)...

Maybe that's pushing it too far (we could use leverage eloquent query scopes for some of this).

That being said, I think enough of the thinking about this is done for it to start.

@poowf poowf deleted a comment from Oppong Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants