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

Installed tailwind, added components page, created toy components with django-components and embedded Vue #4800

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

@ashkonf ashkonf requested a review from albertisfu December 8, 2024 23:37
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Ashkon Farhangi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ashkonf ashkonf requested review from mlissner and albertisfu and removed request for albertisfu December 8, 2024 23:43
@mlissner
Copy link
Member

@ashkonf, can you please sign the CLA per the instructions above?

Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

OK, this PR has a lot of bits to it to make things work, and most of it is so complicated/fragile I can barely figure it out. I think @ERosendo might have some better intuition, since he set up tailwind on Bots.law, but I did make a number of comments for things I noticed.

I think we should approach this PR in two steps:

  1. Focus on cleaning up the various configurations, settings, installations, etc, so they're polished and optimized.

    As part of this, I think we'll want to include the reloader in our docker-compose.yaml file, similar to how big cases does it.

  2. Dial in the implementation of the two components, so we can see the parts that really matter.

@ashkonf, can you work on the comments I've made so far, and @ERosendo, can you review after that, please?

@@ -456,6 +457,10 @@ async def latest_terms(request: HttpRequest) -> HttpResponse:
)


async def components(request: HttpRequest) -> HttpResponse:
return TemplateResponse(request, "components.html", {"private": False})
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this private, actually:

Suggested change
return TemplateResponse(request, "components.html", {"private": False})
return TemplateResponse(request, "components.html", {"private": True})

Comment on lines +92 to +95
path(
"__reload__/",
include("django_browser_reload.urls"),
),
Copy link
Member

Choose a reason for hiding this comment

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

In bots.law, we did this inside the block on line 98 checking if we're testing:

if settings.DEVELOPMENT:
    urlpatterns.append(
        path("__reload__/", include("django_browser_reload.urls")),
    )
    if not settings.TESTING:
        urlpatterns.append(
            path("__debug__/", include("debug_toolbar.urls")),
        )

Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised to see such a large CSS file. Isn't the tailwind idea that we only ship the CSS we use?

@@ -54,6 +54,10 @@

<link href="{% static "css/font-awesome.css" %}" rel="stylesheet">

<link href="{% static "css/app.css" %}" rel="stylesheet">
{% tailwind_css %}
<script src="https://cdn.jsdelivr.net/npm/vue@2/dist/vue.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

We never rely on a CDN this way. We have our own and it's more secure not to trust others. I think it'd probably also fail our CSP rules (but not sure).

@@ -49,6 +49,7 @@
"webpack-dev-server": "^4.10.0"
},
"dependencies": {
"@tailwindcss/cli": "^4.0.0-beta.6",
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to add some of this stuff to our docker-compose file. The Big Cases bot has a good example of how to do it, IIRC. That way, we'll get hot reload and all that fun stuff without having to run extra stuff.

@@ -54,6 +54,10 @@

<link href="{% static "css/font-awesome.css" %}" rel="stylesheet">

<link href="{% static "css/app.css" %}" rel="stylesheet">
Copy link
Member

Choose a reason for hiding this comment

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

I'm finding this a bit weird. We link to app.css, which imports tailwindcss. Gotta be a better way. I think Bots.law just has:

  {% tailwind_preload_css %}
  {% tailwind_css %}

Honestly not sure what the preload vs. regular do, but definitely simpler/cleaner?

@@ -0,0 +1,31 @@
{% extends "base.html" %}
{% load humanize %}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need humanize, do you?

{% load component_tags %}
{% load static %}

{% block title %}Components – CourtListener.com{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
{% block title %}Components – CourtListener.com{% endblock %}
{% block title %}Components Library – CourtListener.com{% endblock %}


{% block content %}
<div class="col-xs-12">
<h1 class="">Components</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<h1 class="">Components</h1>
<h1 class="">Components Library</h1>

Copy link
Member

Choose a reason for hiding this comment

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

Let's add a paragraph to this page explaining what it's for and why it exists.

@mlissner
Copy link
Member

I'm setting this for Eduardo's next sprint, but Ashkon, if you can address my suggestions before then, that'd be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Do
Development

Successfully merging this pull request may close these issues.

4 participants