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

Update home.css for phx.new --no-tailwind #5828

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rhcarvalho
Copy link
Contributor

Pick up template changes (including core components) and Tailwind upstream changes.

The majority of additions are due to picking up classes referenced in core_components.ex which were previously not part of the original home.css installer template.

Used the same Tailwind version as new Phoenix apps to re-generate the CSS file.

Steps to reproduce:

mix phx.new my_app
mix assets.build
cp priv/static/assets/app.css installer/templates/phx_static/home.css (retaining the note at the top of the file)

Steps to test the changes:

mix phx.new my_app2 --no-assets
mix phx.server
(verify that app renders as expected)

@@ -597,14 +624,48 @@ select {
--tw-skew-y: 0;
--tw-scale-x: 1;
--tw-scale-y: 1;
--tw-pan-x: ;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SteffenDE
Copy link
Contributor

@rhcarvalho I was wondering why the new file contains much more classes than the old one, and I think if we really want to update it, we should run tailwind with only the HTML needed for the home page (-> home.html.heex and the flash component) instead of simply copying the app.css. This can be done quite easily using the tailwind playground: https://play.tailwindcss.com/5Rq1Vqs9fg

Here's the generated css: https://gist.github.com/SteffenDE/891897e327e7d5adcb4ef79f390bfe07

If you want, you can copy it from my gist. Then the diff should be minimal.

@rhcarvalho
Copy link
Contributor Author

(I'm on mobile)

Hi! From memory, the changes comprise:

  1. Bringing back deleted but valid parts of Tailwind.
  2. Changes between the Tailwind versions, like different preflight styles.
  3. I think "container" appears in a comment in core_components.ex but is not used as a class, and that pulls some dependencies (this one I remember vividly because I was not super happy introducing so many lines).
  4. Existing CSS could to be missing classes (not sure)

I documented in the commit message (IIRC) the steps I did, and indeed it is not only the home.html.ex we have to look at.

I thought not making manual exceptions and edits would make maintenance easier, perhaps eventually automating generating this file.

Okay from my side if you prefer a different direction! Thanks for having a look and all the work you've been doing 💜

@rhcarvalho
Copy link
Contributor Author

@SteffenDE I'm taking another look at this.

Your suggestion of using the Tailwind Playground was nice, thanks! I don't recall using that tool and it's good to know it's there.

But the approach of pasting code in the playground missed a few things, even when I pasted the full static HTML to render the home page of a new app:

  • Custom tailwind config which is hard to reproduce on the playground

    • In particular, hero-* icons are missing (there are 6 hero-* occurrences in the HTML)
    • Misses phx-* variants like phx-submit-loading
    • Misses everything from @tailwindcss/forms like CSS for button
  • Dynamic classes coming in from core components like classes used in show/2 and hide/2
    Even after copying the full static HTML source into the playground, Tailwind cannot detect some class names because of how they end up in the HTML, example:

    phx-connected="[["hide",{"time":200,"to":"#client-error","transition":[["transition-all","transform","ease-in","duration-200"],["opacity-100","translate-y-0","sm:scale-100"],["opacity-0","translate-y-4","sm:translate-y-0","sm:scale-95"]]}]]"

    The transition-all etc are all missing from the output CSS.

Pick up template changes (including core components) and Tailwind
upstream changes.

The majority of additions are due to picking up classes referenced in
core_components.ex which were previously not part of the original
home.css installer template.

Used the same Tailwind version as new Phoenix apps to re-generate the
CSS file.

Steps to reproduce:

mix phx.new my_app
mix assets.build
cp priv/static/assets/app.css installer/templates/phx_static/home.css
(retaining the note at the top of the file)

Steps to test the changes:

mix phx.new my_app2 --no-assets
mix phx.server
(verify that app renders as expected)
@rhcarvalho
Copy link
Contributor Author

Rebased on top of latest main to fix conflict (caused by removing the antialiased class in #5827) and updated Tailwind version to 3.4.4 (as in #5858; note the Tailwind version caused no change to the generated CSS).

diff
diff --git installer/templates/phx_static/home.css installer/templates/phx_static/home.css
index 363f2522..137c999a 100644
--- installer/templates/phx_static/home.css
+++ installer/templates/phx_static/home.css
@@ -1,7 +1,7 @@
 /* Default styling for the home page, this file can be deleted safely */
 
 /*
-! tailwindcss v3.4.3 | MIT License | https://tailwindcss.com
+! tailwindcss v3.4.4 | MIT License | https://tailwindcss.com
 */
 
 /*
@@ -1526,11 +1526,6 @@ select {
   color: rgb(24 24 27 / var(--tw-text-opacity));
 }
 
-.antialiased {
-  -webkit-font-smoothing: antialiased;
-  -moz-osx-font-smoothing: grayscale;
-}
-
 .opacity-0 {
   opacity: 0;
 }

Comment on lines +725 to +757
.container {
width: 100%;
}

@media (min-width: 640px) {
.container {
max-width: 640px;
}
}

@media (min-width: 768px) {
.container {
max-width: 768px;
}
}

@media (min-width: 1024px) {
.container {
max-width: 1024px;
}
}

@media (min-width: 1280px) {
.container {
max-width: 1280px;
}
}

@media (min-width: 1536px) {
.container {
max-width: 1536px;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are notably accidental, as the word "container" appears in attr docs in core_components.ex.

Considering how Tailwind works with heuristics to eliminate unused classes, I'd rather follow an easy to reproduce procedure that produces all necessary classes, than cherry pick and risk missing classes.

Comment on lines +759 to +770
.hero-arrow-left-solid {
--hero-arrow-left-solid: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" fill="currentColor" aria-hidden="true" data-slot="icon"> <path fill-rule="evenodd" d="M11.03 3.97a.75.75 0 0 1 0 1.06l-6.22 6.22H21a.75.75 0 0 1 0 1.5H4.81l6.22 6.22a.75.75 0 1 1-1.06 1.06l-7.5-7.5a.75.75 0 0 1 0-1.06l7.5-7.5a.75.75 0 0 1 1.06 0Z" clip-rule="evenodd"/></svg>');
-webkit-mask: var(--hero-arrow-left-solid);
mask: var(--hero-arrow-left-solid);
-webkit-mask-repeat: no-repeat;
mask-repeat: no-repeat;
background-color: currentColor;
vertical-align: middle;
display: inline-block;
width: 1.5rem;
height: 1.5rem;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heroicons were all originally missing and are not picked up by the Tailwind Playground because we embed them using a custom plugin.

Some icons do get used in the static HTML for the home page:

image

Comment on lines +824 to 834
.sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
white-space: nowrap;
border-width: 0;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another "accidental" addition that is part of core_components.ex but not used in home.html.heex in particular. This one is used in the table component.

Comment on lines +856 to 859
.-inset-y-px {
top: -1px;
bottom: -1px;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another one used in the table component, not in home... and so will be the case for some other classes.

There's a decision whether it's more important to have all necessary classes and perhaps a few false positives (Tailwind's approach), or trying to have a minimal CSS file and perhaps missing a few classes and having to keep the diligence when making future updates.

Considering we've missed classes for a long time and nobody complained, I think either way doesn't really matter. This CSS file is only used for projects without the assets pipeline, and the very next thing after creating the app is changing the HTML template and CSS anyway -- it possibly doesn't matter that icons are missing or small things are slightly off.

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