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 file field support #53

Merged
merged 5 commits into from
Jun 1, 2021
Merged

Conversation

bcvandendool
Copy link
Contributor

Ive added support for clearable file field widget, and updated the file field test case to pass with the new setup. It is mostly a port from the bootstrap4 code, i believe i have changed everything to the bootstrap 5 equivalent, but i might have missed something.

@smithdc1
Copy link
Member

Thanks for the PR. 👍

Could I ask you to have a look at how the tests were written for #44?

We're trying to improve the tests by asserting the whole field rather than html snippets. It will also make it far easier to review. Could you have a look at that?

@bcvandendool
Copy link
Contributor Author

Ive changed the tests to be similar to those.

@smithdc1 smithdc1 closed this May 31, 2021
@smithdc1 smithdc1 reopened this May 31, 2021
@bcvandendool
Copy link
Contributor Author

Ive fixed the formatting issue, sorry about that.

@smithdc1
Copy link
Member

No problem. I hadn't realised until now that I hadn't setup the tests to run from forks. 🕵️

I've had a quick glance at your patch this morning and it looks great. I just want to have a bit of a play as there's a few different scenarios here.

@smithdc1
Copy link
Member

Sorry for the incremental review, trying to make progress where I can.

I'm seeing the help text rendering twice. Here's an image and here's a demo bootstrap5 test site here showing the error.

I think it's coming from the help text being rendered both in the field.html and field_file.html, but don't have time to look more closely right now.

image

@bcvandendool
Copy link
Contributor Author

You were right, it was indeed added in both field.html and field_file.html, ive fixed it by removing it from field_file.html as there is no need for special handling of the help text in this case as far as i understand.

@smithdc1 smithdc1 linked an issue May 31, 2021 that may be closed by this pull request
@smithdc1
Copy link
Member

smithdc1 commented Jun 1, 2021

Thanks for the updates here.

I'm not seeing the error message when the field fails validation. Appologies for the upcoming blobs of html. I think it would be useful to also add a test for the failing version of this field?

image

We're currently getting

<div id="div_id_default_form_failing-file_field" class="mb-3">
   <label for="id_default_form_failing-file_field" class="form-label requiredField">
   File field<span class="asteriskField">*</span> </label> 
   <div class=" mb-2">
      <div class="input-group mb-0  is-invalid"> <input type="file" name="default_form_failing-file_field" class="form-control is-invalid" id="id_default_form_failing-file_field" required=""></div>
   </div>
   <span id="error_1_id_default_form_failing-file_field" class="invalid-feedback"><strong>This field is required.</strong></span> 
</div>

I think we need

<div id="div_id_default_form_failing-file_field" class="mb-3">
   <label for="id_default_form_failing-file_field" class="form-label requiredField">
   File field<span class="asteriskField">*</span> </label>
   <div class=" mb-2">
      <div class="input-group mb-0  is-invalid"> <input type="file" name="default_form_failing-file_field" class="form-control is-invalid" id="id_default_form_failing-file_field" required=""></div>
      <span id="error_1_id_default_form_failing-file_field" class="invalid-feedback"><strong>This field is required.</strong></span>
   </div>
</div>

@smithdc1 smithdc1 linked an issue Jun 1, 2021 that may be closed by this pull request
And add test for file field validation
@bcvandendool
Copy link
Contributor Author

Ive now moved the help and error handling to the file_field.html, im not sure if the way i handled it in field.html is the cleanest though. Ive also added tests for failing validation of the file fields.

@smithdc1
Copy link
Member

smithdc1 commented Jun 1, 2021

This looks great. Thank you very much!

@smithdc1 smithdc1 changed the title Add clearable file field support Add file field support Jun 1, 2021
@smithdc1 smithdc1 merged commit cb2fc3f into django-crispy-forms:main Jun 1, 2021
@smithdc1 smithdc1 added this to the Next Release milestone Jun 3, 2021
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.

Bootstrap5 clearable file input add script to external script file to handle SCP
2 participants