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

Add limit to a custom layout #200

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Petoches
Copy link

@Petoches Petoches commented Jul 11, 2020

Add a feature to limit occurence of a custom layout.

/**
 * The layout's limit number
 *
 * @var int
 */
protected $limit = 1;

add this property to your custom layout to limit its use on a flexible field.
Trying to add a layout when the limit is reached will make a toast message error.

toast limit

if this property isn't set then the layout doesn't have any limit.

@Petoches Petoches mentioned this pull request Jul 11, 2020
Copy link
Member

@toonvandenbos toonvandenbos left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your PR!

There are just a few changes I'd like to see implemented before merging it.

Also, your way of counting the layouts in the field's Vue component does not seem bug-free. You shouldn't decrement or increment the layoutLimit each time a layout is added or removed. There are a few reasons for this :

  1. When a flexible field limits a certain layout to 3 occurrences and is loaded with an existing value (on update), which already contains 3 occurrences of the said layout, we'll still be able to add 3 more since the initial limit value still contains 3.
  2. If for some reason an external package modifies the field, we could have a problem with simply incrementing or decrementing the initial limit value.

Instead I would leave the initial limit value where it is (in this.field.layouts) and use your counter this.layoutLimit (which I would rename to layoutsCounter)as an actual representation of the real layouts count, which is updated each time something is changed. For example, we could extract this behavior in a function, something like this:

updateLayoutsCount() {
    let key, layout;

    this.layoutsCounter = {};
    
    for (key in this.groups) {
        layout = this.groups[key].name;

        this.layoutsCounter[layout] = (this.layoutsCounter[layout] === undefined)
            ? 1
            : this.layoutsCounter[layout] + 1;
    }
}

You could then call this function after the component has been set up and each time a group has been successfully added or removed.

Before a new group gets added, you could check the layout's counter value against the original limit in this.field.layouts.

Let me know what you think!

Have a great day.

resources/js/components/FormField.vue Outdated Show resolved Hide resolved
resources/js/components/FormField.vue Outdated Show resolved Hide resolved
resources/js/components/FormField.vue Outdated Show resolved Hide resolved
resources/js/components/FormField.vue Show resolved Hide resolved
@Petoches
Copy link
Author

Petoches commented Jul 12, 2020

Hello,

When a flexible field limits a certain layout to 3 occurrences and is loaded with an existing value (on update), which already contains 3 occurrences of the said layout, we'll still be able to add 3 more since the initial limit value still contains 3.

it works even on update resource, when you're on update the package still call the method addGroup and thus the layout limit is taken, i tested it before making the pr and it works well.

If for some reason an external package modifies the field, we could have a problem with simply incrementing or decrementing the initial limit value.

My code doesn't incremente or decrement the initial limit value, it increment and decrement an array of values based on those limits.

let layoutCounter = []; this.field.layouts.map(function(value, key) { layoutCounter[value.name] = value.limit; });

this.layoutCounter[layout.name]++;

i'll add the change asap

@Broutard
Copy link
Contributor

Any news about this ?

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

Successfully merging this pull request may close these issues.

3 participants