-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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
Fixed #35461 -- Added detail in django tutorial step 8 so that django… #18285
Fixed #35461 -- Added detail in django tutorial step 8 so that django… #18285
Conversation
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.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
11ab1e4
to
6e75e10
Compare
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.
Thank you for this @RosanaRufer 👍
Really liked how you linked your reasoning doc 😁 I have a couple of thoughts
docs/intro/tutorial08.txt
Outdated
To see the toolbar in the polls application make sure all your templates | ||
are using `complete HTML documents <https://developer.mozilla.org/en-US/docs/Learn/HTML/Introduction_to_HTML/Getting_started#anatomy_of_an_html_document>`_. | ||
The toolbar will only render in responses that contain the ``<body></body>`` tags. |
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.
We can have this in an ... admonition::
or we can remove this paragraph entirely?
In your reasoning docs, you mention
The main goal of this section is to learn how to add a package in the first place and a bit of django-debug-toolbar in second place.
It's a good point, after telling them they can see this on the polls app we don't encourage them to do anything with this information (such as checking a query).
This information is certainly secondary (they don't have to do this) and possibly not worth the headspace? What do you think?
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.
It's a good point. After pondering about it, it might be a good idea to remove it. As the tutorial is already a bit verbose. Better to remove verbosity and leave this information to the plugin's 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 think it's a reasonable question for people to ask when going through the tutorial of "Why does the toolbar not show up for the application I've built?". The toolbar doesn't make it super obvious that you need a </body>
tag since you need to go to the reference documentation to find that.
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.
Done in fe1ac7e
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 think it's a reasonable question for people to ask when going through the tutorial of "Why does the toolbar not show up for the application I've built?". The toolbar doesn't make it super obvious that you need a tag since you need to go to the reference documentation to find that.
I agree it's a reasonable question.
I like to think of it this way... for this tutorial, which is often the first time someone uses Django, we are designing the journey for the reader. In this designed journey, they don't need to worry. The less information/worries the better
Curious readers can google and explore on their own
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 understand the reasoning to keep the tutorials/documentation as slim as possible and people can eventually figure out why. I don't agree with it though because it's not a clear reason that can be quickly searched for on the internet.
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.
Should be an admonition for sure
My first google of "why isn't django-debug-toolbar showing" had the relevant docs from django-debug-toolbar appear 🤷♀️ but I know the issue and what the fix is. Maybe other people would google differently
Saw someone on reddit ask this question of the tutorial and there's the ticket raiser too, but we previously told them to go to the polls application and that it would be visible there. Hard to know if anyone would do that without us telling them to
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.
If you'd like to leave it off here, I'll add it to the toolbar's docs to explain what's going on.
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 let's proceed with this for now, an admonition can be added at a later date if required 👍
The DjDT "handle" isn't visible on the polls application as the templates are missing <body> tags for brevity.
fe1ac7e
to
df40b92
Compare
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.
Thank you @RosanaRufer ⭐ welcome onboard ⛵
One solution for https://code.djangoproject.com/ticket/35461
Trac ticket number
ticket-35461
Branch description
Checklist
main
branch.