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

Added documetation & credential property #214

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

Conversation

aDramaQueen
Copy link
Contributor

Hi,
since I had to work through the JavaScript for Issiue #213, I have this weird behavior writing documentation for undocumented code. So, here we are. This is basically what it is, to this pull request.

But there is one tiny bit more...
I added a new property to the default setting. The credentials property. This is a important security property (see: here), you may give the user the controll over this paramater. I gave it the default value: same-origin. This means, the code didn't change at all. But the user is now able to controll this.

Thats it. Thanks.

Added documentation.
Added new property for settings: credentials.
Added documentation.
Added new property for settings: credentials.
@aDramaQueen aDramaQueen mentioned this pull request Apr 11, 2023
@trco trco self-requested a review May 1, 2023 15:58
@trco trco added the discussion Further information is requested label Jul 1, 2023
@trco
Copy link
Owner

trco commented Jul 1, 2023

@aDramaQueen This is a great idea and I would like to discuss potential to extend it even further.

  1. What do you think about allowing user to control all the options for the fetch() method? https://developer.mozilla.org/en-US/docs/Web/API/fetch
  2. And what about Bootstrap 4 version? There is also a potential to allow user to completely control ajax calls https://github.com/trco/django-bootstrap-modal-forms/blob/master/bootstrap_modal_forms/static/js/jquery.bootstrap.modal.forms.js#L35.

@aDramaQueen
Copy link
Contributor Author

To 1.) Yes, absolutely. That would only be consequential.
To 2.) There was a reason, why I only touched the Bootstrap 5 Code. I don't now jQuery very well.

A design decision must then be made here. Should the user set these options in:

  • JavaScript
  • Python

First one would be easier to implement. You basically write & pass the JavaScript object.
Second one would be more conveniant since you would stay in Python, but it would require some kind of exchange. You need to inject this information. I would suggest a custom Django - Tamplate Tag:

<div class="modal fade" tabindex="-1" role="dialog" id="modal">
  {% my-custom-tag %}
  <div class="modal-dialog" role="document">
    <div class="modal-content"></div>
  </div>
</div>

With this, you could inject another div, that would hold all necessary data.

@trco
Copy link
Owner

trco commented Sep 2, 2023

@aDramaQueen I believe JavaScript option would be more reasonable, since user would be able to set everything when initialising modal form with modalForm().

Any chance that you update this PR and prepare Boostrap5 implementation? I would then add it for Bootstrap4 afterwards. As always your contribution would be highly appreciated.

Thanks for great improvement idea.

trco and others added 20 commits November 30, 2023 18:09
* Fix waring from BSModalDeleteView / DeleteMessageMixin with Django 4

* fix when form has field named method

* add new mixin for FormValidation, remove  SuccessMessageMixin dependecy

* Updated project to current Django LTS version

Updated deprecated "is_safe_url" function to "url_has_allowed_host_and_scheme".
Added automatically database population, if DB is empty.
Updated requirements to current version.
Updated settings.
Removed outdated version support.
Updated gitignore.

* Minor refactoring

* Updated Project

Added type hints.
Updated test cases.
Removed last remaining snippets for outdated Django versions.

* Removed unused constant

* Updated required version

* Minor Bugfix

Removed __slots__ from dataclass, to match Python 3.8 support.

* refactor save method in CreateUpdateAjaxMixin

* remove compatibility.py

* remove utils.py

* remove types

* add is_ajax method and remove imports

* remove unneeded class inheritence

* remove obsolete class name parameter

* revert examples to version in master branch

* remove static folder

* remove types from tests

* remove unneeded comments

* update get and set for form action and method attributes

* update bootstrap5.modal.forms.min.js

* update assert string to pass the test

* update DeleteMessageMixin comment

* cleanup .gitignore

---------

Co-authored-by: Christian Wiegand <[email protected]>
Co-authored-by: Mark Monaghan <[email protected]>
Co-authored-by: aDramaQueen <[email protected]>
Bumps [django](https://github.com/django/django) from 3.2 to 3.2.18.
- [Release notes](https://github.com/django/django/releases)
- [Commits](django/django@3.2...3.2.18)

---
updated-dependencies:
- dependency-name: django
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…o: (trco#219)

If return class property success_url if exists, if not return value from classes get_success_url method. If that doesn't exist return value from models get_absolute_url method.
* Fix redirect of FormValidationMixin to be compliant to standard django:
If return class property success_url if exists, if not return value from classes get_success_url method. If that doesn't exist return value from models get_absolute_url method.

* Fix success url to work with all constellations
…trco#220)

Change the success message so that it can be used with either a static message (class property) or a dynamic success message from get_success_message method (so we're able to use model instance attributes in the message) or even without a message.

Co-authored-by: Uroš Trstenjak <[email protected]>
Bumps [django](https://github.com/django/django) from 4.2 to 4.2.1.
- [Commits](django/django@4.2...4.2.1)

---
updated-dependencies:
- dependency-name: django
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Jeffrey McAthey and others added 8 commits November 30, 2023 18:09
Bumps [django](https://github.com/django/django) from 4.2.1 to 4.2.3.
- [Commits](django/django@4.2.1...4.2.3)

---
updated-dependencies:
- dependency-name: django
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@aDramaQueen
Copy link
Contributor Author

Sorry for the delay, there was a lot to do at work.

I re-based the branch and added the changes. This should make merging easier.

Now all Fetch API parameters are set to the recommended default values. I also added the changes to the README.rst.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Further information is requested next release?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants