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

Review chapter 08 prettification #196

Merged
merged 9 commits into from
Oct 30, 2023

Conversation

Xronophobe
Copy link
Collaborator

@Xronophobe Xronophobe commented Oct 20, 2023

Nice, faster-paced chapter, easy to follow and seeing the visual upgrade on the Lists page is a very rewarding feeling.

I was wondering whether Bootstrap is still a future-proof choice, considering the changes at Twitter, but it seems like it is: https://github.com/orgs/twbs/discussions/38986

ps.: I just noticed we are going with Python 3.12 now. I was using 3.11 for reviewing the chapter, but I think it shouldn't be a problem.

@@ -971,7 +1012,7 @@ INSTALLED_APPS = [
"django.contrib.sessions",
"django.contrib.messages",
"django.contrib.staticfiles",
"lists",
"lists.apps.ListsConfig",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw my INSTALLED_APPS have single quote entries, why are these double quotes? Maybe some auto-formatter?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i ran black on everything at some point. and i think django does too now, so everything in settings.py has double quotes these days...

(also i change to just using "lists" rather than lists.apps.ListConfig because the latter just seems needlesscly complicated.

@Xronophobe Xronophobe force-pushed the review--chapter-08-prettification branch from 666779d to ed2c7b2 Compare October 20, 2023 11:33
@@ -822,6 +859,7 @@ All that took me a few goes, but I'm reasonably happy with it now
[[homepage-looking-better]]
.The lists page, looking good enough for now...
image::images/prettified-final.png["Screenshot of lists page in light mode with decent styling."]
// CSANAD: This image seemed to be broken, so I uploaded a new one.
Copy link
Collaborator Author

@Xronophobe Xronophobe Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the resolution, my image is significantly smaller in size.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks Csanad!

@Xronophobe Xronophobe marked this pull request as ready for review October 20, 2023 13:22
@hjwp hjwp merged commit 99690f2 into hjwp:main Oct 30, 2023
1 check failed
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.

2 participants