-
Notifications
You must be signed in to change notification settings - Fork 318
more structured quick start #542
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #542 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 2 2
Lines 8 8
=====================================
Hits 8 8Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #542 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 2 2
Lines 8 8
=====================================
Hits 8 8Continue to review full report at Codecov.
|
codingjoe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mfrasca thank you for your contribution. Documentation improvements are always much appreciated. Let me know if you have further questions. I am happy to merge this, once you made some minor adjustments.
docs/Makefile
Outdated
| @@ -0,0 +1,19 @@ | |||
| # Minimal makefile for Sphinx documentation | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this is related to your PR. Please drop the chance. If you want to build the docs, please use python setup.py build_sphinx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how this is related to a documentation PR … without the Makefile, and without attempting python setup.py build_sphinx, it was unpractical to suggest corrections in the documentation. some are purely typesetting, and wanted to make sure that sphinx understood the formatting, because part of the output was a bit out of sync.
do you insist, in not having the Makefile in the docs? it doesn't do any harm, does it? anyway, I will double check that the choice is properly documented ("if you want to build the docs, please use python setup.py build_sphinx).
docs/get_started.rst
Outdated
| * are using Python3, | ||
| * set up a virtual environment with ``python3 -m venv <location>``, | ||
| * activated the virtual environment, | ||
| * installed ``django`` using ``pip``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Django is a dependency of this package. So you don't necessarily need to install it. However, you do need an app setup.
Also, you may use pipenv, pip is not hard requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't ever heard of pipenv, but I read »The official recommended way [to install pipenv] is to use pip«.
assumption: »you have a Django app«?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pipenv is a whole toolset. You can use it to describe the entire environment including checksums, to ensure your environments are the same across machines. You should try it out!
Besides that, maybe better: you have a running Django app
docs/get_started.rst
Outdated
| Assumptions | ||
| ----------- | ||
|
|
||
| * You have a Django project, possibly with models linked by foreign key fields, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the main scenario, but not all of them. Maybe it makes sense to coin it, so it is clear that the library does support other scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I"m not sure I understand your comment. what about you edit the assumptions as to make them more correct, after you accept the PR?
| class MyForm(forms.Form): | ||
| things = ModelMultipleChoiceField(queryset=Thing.objects.all()) | ||
| from django import forms | ||
| from django_select2 import forms as s2forms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call to include the imports. I always forget 👍
| things = ModelMultipleChoiceField(queryset=Thing.objects.all(), widget=Select2MultipleWidget) | ||
| widgets = { | ||
| 'category': s2forms.Select2Widget, | ||
| 'author': s2forms.ModelSelect2Widget(model=auth.get_user_model(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did not import auth, I would also suggest to import get_user_model directly instead, since you don't use anything else from the auth module.
| class MyForm(forms.Form): | ||
| things = ModelMultipleChoiceField(queryset=Thing.objects.all(), widget=Select2MultipleWidget) | ||
| widgets = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include the entire code example, otherwise people will be confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't written it yet. I'll review the PR in its entirety, and come back to you soon (36h).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
36h went by, and I'm still busy on the project I'm working at. (mfrasca/ghini)
sooner or later, from that project, I will again need to dive into djangoSelect2, and I will again read the docs and amend the docs where what I will be reading doesn't make sense to me.
in particular, I will need the Multiple select widget, which I've not yet used.
codingjoe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I like how much effort you are putting into this. I absolutely appreciate it. Good documentation is key to open source.
|
|
||
| $('.django-select2').djangoSelect2({placeholder: 'Select an option'}); | ||
|
|
||
| Please replace all your ``.select2`` invocations with the here provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Please replace all your ``.select2`` invocations with the here provided | |
| Please replace your ``.select2`` invocations with the here provided |
docs/developers.rst
Outdated
| @@ -0,0 +1,15 @@ | |||
| Hints to Developers | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, good idea to add that. Please a CONTRIBUTING.rst file to the repo root and include it in here.
The contributing file is also supported by GitHub and will be displayed when someone opens a PR.
.gitignore
Outdated
| @@ -1,3 +1,4 @@ | |||
| *~ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a emacs user, I put this one everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I prefer to only have language or build specific things the .gitignore file. If you have things that are specific to your local environment, please put them into your global gitignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did not know of this option. thank you, it's useful.
Co-Authored-By: Johannes Hoppe <[email protected]>
Co-Authored-By: Johannes Hoppe <[email protected]>
I've been trying to get django-select2 to work in my setup, but there's something missing and I don't quite understand what it is. in an attempt to understand things more clearly, I've written a less quick quick start, which was very much 'kort door de bocht' (that's Dutch, but the English "blunt" doesn't quite give the impression).
first of all, I've put back the missing sphinx Makefile.
I've added an Assumptions section before the Installation instructions.
next, I've given three examples, the first one works fine, the second almost, the third I've not completed. I think that the three of them are useful, for they exemplify three different situations.
I've removed the alternative style, defining fields directly in the class: I have not managed to get this to work at all.
I will open an issue about "the second almost" works.