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

Bootstrap 4 ListField / ListAddField Always Tries to Set Count to 1 #875

Closed
LordA98 opened this issue Feb 16, 2021 · 5 comments · Fixed by #907
Closed

Bootstrap 4 ListField / ListAddField Always Tries to Set Count to 1 #875

LordA98 opened this issue Feb 16, 2021 · 5 comments · Fixed by #907
Assignees
Labels
Type: Bug Bug reports and their fixes
Milestone

Comments

@LordA98
Copy link

LordA98 commented Feb 16, 2021

Hi,

I'm using the bootstrap-4 theme. When I use the default ListItem component or a custom ListItem component, clicking 'Add' the first time always tries to set the count to 1.

For example, if the initialCount=0, clicking add once adds one ListItemField. This works as expected.

However, if the initialCount=1, clicking add once does nothing, clicking add again adds a second ListItemField.

If the initialCount=2, clicking add once removes the second ListItemField so that there is only 1. Clicking it again then adds a second ListItemField back.

This occurred using this code:

<ListField
    name="prices"
    addIcon={<Add />}
    removeIcon={<Delete />}
    initialCount={1}
    showInlineError
/>

It was also happening for my custom list field, but I'm assuming the fix will apply to both.

@radekmie radekmie added the Type: Bug Bug reports and their fixes label Feb 19, 2021
@radekmie
Copy link
Contributor

Hi @LordA98. Thanks for the report! @Monteth: could you take a look at it?

Monteth added a commit that referenced this issue Feb 22, 2021
@Monteth
Copy link
Member

Monteth commented Feb 22, 2021

Hi @LordA98,
I tried to reproduce your issue on this branch, but I did not succeed.
Could you introduce more details, or just change my reproduction environment to fit your case?

@LordA98
Copy link
Author

LordA98 commented Feb 27, 2021

Hi @Monteth,

Sorry for the delayed response.

I will take a look at the reproduction environment now.

@LordA98
Copy link
Author

LordA98 commented Feb 27, 2021

Okay, it seems to be caused by the requiredByDefault flag (at least for me anyway).

This is my form:

<AutoForm schema={ItemBridge}>
    <ListField name="options" addIcon="+" initialCount={1} />
    <SubmitField />
</AutoForm>

This is my schema:

ItemSchema = new SimpleSchema(
  {
    options: { type: Array, label: "Option Groups" },
    'options.$': String,
  },
  { requiredByDefault: false }
);

const ItemBridge = new SimpleSchema2Bridge(ItemSchema);

export { ItemBridge, ItemSchema };

Removing the requiredByDefault flag at the bottom "fixes" the issue.

So, is this related to Uniforms or to Simpl Schema?

@radekmie
Copy link
Contributor

radekmie commented Mar 8, 2021

Hi @LordA98. I've dug into it today, and it's indeed hard to debug. So, let me describe the problem first and then how to fix it.

  1. All fields in SimpleSchema are required by default. For us, the important part is, that it implies this condition to be true.
    • Using requiredByDefault: false (globally) or optional: true (on the field), makes it optional. And, of course, the aforementioned condition is false.
  2. During the first render, uniforms resolve all initial values. It means that the fields will populate the model, using their default values (and these received through props). The code is here. The important part is that it happens only if the field is required.
  3. Then, the actual culprit is that the ListAddField, that actually adds new items, is not aware of its parent default value here. And why? Because it calculates it without the initialCount, used in the schema.

Now, most probably adding initialCount to the second argument of useField in the ListAddField will fix it. We have to check it and if so, release a fix. However, if you have a custom list field - just give it a try.

@Monteth: Could you add a unit test to verify this problem and this solution? 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug reports and their fixes
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants