-
Notifications
You must be signed in to change notification settings - Fork 6
Switch to tailwind #162
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
Switch to tailwind #162
Conversation
- Remove Webpack (woot!) - Add Hyperscript for the navigation menu
- Rev the minor version - Add a description - Update GitHub links
- Menu toggle is handled by custom JS
I also added a commit to remove Hyperscript. It seems like overkill and an unnecessary dependency for now. If it becomes useful, we'll add it back but seems odd to add a huge dependency instead of 3 lines of JS. |
- Move under the pythonsd/ directory where the template that uses it lives - Remove copying static files in package.json. Not needed after moving the main.js - Remove logging when debug false (can't get errors otherwise)
Co-authored-by: Jeremy <[email protected]>
This is more of a general conversation but it could be grouped into these changes: Do we want |
Co-authored-by: Jeremy <[email protected]>
Co-authored-by: Jeremy <[email protected]>
I don't feel particularly strong but I generally don't open links in new windows unless there's a reason or an expectation that that should happen. |
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.
Only minor suggestions based on the external link conversation. The switch to Tailwind looks good. Good call removing Hyperscript for a single element.
Co-authored-by: Jeremy <[email protected]>
Co-authored-by: Jeremy <[email protected]>
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.
Overall, this looks great! None of these suggestions are necessary. Just minor formatting stuff. This looks ready to merge as-is.
<div id="navigation-toggle" class="h-6 w-6 cursor-pointer md:hidden block text-slate-100"> | ||
<svg xmlns="http://www.w3.org/2000/svg" id="menu-button" fill="none" viewBox="0 0 24 24" stroke="currentColor"> | ||
<path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M4 6h16M4 12h16M4 18h16"></path> | ||
</svg> | ||
</div> | ||
|
||
<div id="navigation-menu" class="hidden w-full mt-4 md:w-auto md:flex md:items-center md:mt-0"> | ||
<ul class="md:flex md:justify-between font-extrabold mb-0"> | ||
<li class="py-2 md:mr-5"> | ||
{% url 'code-of-conduct' as conduct_url %} | ||
<a class="text-slate-100 hover:text-slate-300" href="{% if request.path != conduct_url %}{{ conduct_url }}{% else %}#{% endif %}">Code of Conduct</a> | ||
</li> | ||
<li class="py-2 md:mr-5"> |
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.
<div id="navigation-toggle" class="h-6 w-6 cursor-pointer md:hidden block text-slate-100"> | |
<svg xmlns="http://www.w3.org/2000/svg" id="menu-button" fill="none" viewBox="0 0 24 24" stroke="currentColor"> | |
<path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M4 6h16M4 12h16M4 18h16"></path> | |
</svg> | |
</div> | |
<div id="navigation-menu" class="hidden w-full mt-4 md:w-auto md:flex md:items-center md:mt-0"> | |
<ul class="md:flex md:justify-between font-extrabold mb-0"> | |
<li class="py-2 md:mr-5"> | |
{% url 'code-of-conduct' as conduct_url %} | |
<a class="text-slate-100 hover:text-slate-300" href="{% if request.path != conduct_url %}{{ conduct_url }}{% else %}#{% endif %}">Code of Conduct</a> | |
</li> | |
<li class="py-2 md:mr-5"> | |
<div id="navigation-toggle" class="h-6 w-6 cursor-pointer sm:hidden block text-slate-100"> | |
<svg xmlns="http://www.w3.org/2000/svg" id="menu-button" fill="none" viewBox="0 0 24 24" stroke="currentColor"> | |
<path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M4 6h16M4 12h16M4 18h16"></path> | |
</svg> | |
</div> | |
<div id="navigation-menu" class="hidden w-full mt-4 sm:w-auto sm:flex sm:items-center sm:mt-0"> | |
<ul class="sm:flex sm:justify-between font-extrabold mb-0"> | |
<li class="py-2 sm:mr-5"> | |
{% url 'code-of-conduct' as conduct_url %} | |
<a class="text-slate-100 hover:text-slate-300" href="{% if request.path != conduct_url %}{{ conduct_url }}{% else %}#{% endif %}">Code of Conduct</a> | |
</li> | |
<li class="py-2 sm:mr-5"> |
Ignore this if there are plans to add a third link.
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 I'm going to skip this change. There aren't plans to add a 3rd top navigation link but I also want to keep it simple if we do decide to change it. I'm also not particularly worried if users with screens narrower than 768px have to toggle the nav button.
<div> | ||
<h2>Latest video</h2> | ||
|
||
<div id="youtube-widget" hx-get="{% url 'recent_videos' %}" hx-trigger="load"> |
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.
The two hx-get
requests could be combined into a single request using hx-swap-oob. This would require a third view that calls the other two. Probably not necessary unless there's concern over bandwidth.
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 am not particularly concerned about bandwidth and the overhead of a 2nd request to the same domain in HTTP2 (used in prod) is pretty minimal. For now, I think the current setup is simpler, allows the views to fail/succeed independently, and easily allows them to be cached separately.
|
||
<main role="main"> | ||
{% block main %}{% endblock main %} | ||
</main> | ||
|
||
{# FOOTER #} | ||
<footer class="container"> | ||
<footer class="container mx-auto"> |
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.
(Personal preference) I liked the old footer better on larger displays (more compact) but the new format looks better on mobile. (stacked lines and centered icons).
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.
This is fair. I think I like the old footer as well on larger displays although I'm not particularly worried about how compact it is. I think I like the look of the new icon spacing a bit better in the new footer but I don't love the spacing of text in the new footer.
I wonder if we could switch it to 2 text sections instead of 3. In reality, "edit on github" and the version could be condensed into one as they are related. See screenshots. Thoughts?
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 merged this PR without changing this but I'd be happy to do a separate PR with this if you think this is an improvement.
Co-authored-by: Jeremy <[email protected]>
Co-authored-by: Jeremy <[email protected]>
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.
Love this! Thanks @davidfischer for simplifying and getting rid of webpack 😉
This PR will switch the static assets from Bootstrap 4 to TailwindCSS 3. The actual site will still look pretty similar.
, and Hyperscript (used to expand/collapse the navigation menu on mobile). Hyperscript could be completely removed if we want to just have a few lines of custom JS to power the menu.From a network transfer size, CSS difference (before zipping) is 143.7kb with Bootstrap vs. 7.9kb with Tailwind. JS bundle difference is 212.8kb with Bootstrap+HTMX vs. 148.5kb with HTMX+Hyperscript. Including gzipping, we're looking at 90.9kb with Bootstrap vs. 45.1kb with Tailwind for about half the total network transfer.As a result, the site should load faster especially on slower network connections with these changes. The total gzipped transfer will be 20.0kb with these changes vs. 90.9kb before.Testing it out
git pull && git checkout davidfischer/switch-to-tailwind
npm run install && npm run build
./manage.py runserver
Note: don't forget to rebuild the old static assets when you switch to another branch.
Screenshots
Fixes #153.