Refactor flashes [WIP - Questions]

[patriciomacadden]

The code that displays flashes is repeated a few times in the scaffold and authentication generators.

When you run the generators, you normally end up extracting those lines into a separate partial. This PR aims to simplify that process by creating the app/views/application/_flashes.html.erb partial that is shared for the entire app when running rails g authentication or rails g scaffold .... Once you have it, you can customize it to your needs and the next time the generators are run, you'll be prompted to leave it or overwrite it, etc.

An alternative to this (creating the file when running the generators) would be to create the partial when running rails tailwindcss:install and insert it right into the layout. tbh I like this approach better, but if we go that way we have a few issues:

1. the tailwindcss:install task adds the flex class to the layout's main container, so we'd have to remove it. Not sure why it's added in the first place but it might be a problem.
2. the session and passwords views have the w-2/3 class, meaning the flash would be left-aligned while the session/password form would be centered. we'd end up adding a condition to the flash like "make it w-2/3 and mx-auto if it's the session or passwords controller", which is far from ideal.

What do you think?

The whole point of this PR and my previous ones are to make the scaffold generator as "ready to use" as possible without many modifications. Hope it makes sense 🙂

[flavorjones]

Just a couple of thoughts:

In this and a few other PRs, the class values have been re-ordered (I imagine with some kind of editor plugin like prettier). This makes the PRs very noisy and hard to read, so it's harder to give feedback on what's meaningfully changed. I'm generally in favor of ordering them sensibly, but can you either avoid committing those changes to PRs, or else just give us one big PR with all the class ordering changes in it?

I'm not sure why flex is added to the primary layout. If you think it should be removed to make other things easier, I'm open to it, but we need to document it and maybe think about how to make it easy for people to add if they think it's helpful.

I think I like the idea of having a _flashes partial, and making it part of the site layout. I think it should be rendered with a consistent style regardless of how the page content is rendered, though. Why not just center the flash content? Maybe I'm misunderstanding the issue with flash alignment and content width, and if so I apologize.

[patriciomacadden]

Completely agree on the class ordering thing - I've turned it off and removed all the reorderings. I think i mentioned in another PR but I'm using a vscode plugin and I'm not expecting anyone else to use it, just doesn't make sense. So turning it off is the sensible thing to do.

I'm not sure why flex was added in the primary layout either and I'm not sure what it tries to accomplish. but it makes new views weird. For example, if I add 2 small divs to the index view:

<div>#1</div>
<div>#2</div>

<div class="w-full">
<!-- ... and the rest of the view -->

I'll get this:

which is weird if I'm expecting to get something like this:

To obtain that behavior, I had to wrap everything in another div:

<div class="w-full">
  <div>#1</div>
  <div>#2</div>

  <div class="w-full">
  <!-- ... and the rest of the view -->

Which can be avoided if we don't use the flex.

If we render the flashes in the layout (which is why I prefer too), then since we have the flex class we'll get something like:

if we remove the flex class, then it looks misaligned, because the sign in form (and new/edit password) are mx-auto w-2/3.

I can center the flash, I but didn't want to change its design drastically.

So, by keeping it at the view level instead of layout level, we keep the flash left-aligned to parent container.

So I'm not sure. In one hand, I'd prefer to generate the flashes partial when installing and adding it to the layout, but on the other hand I don't want to mess up the design or remove the flex class (although I'm not 100% convinced we need it - need to investigate).

What do you think?

[patriciomacadden]

this is the PR that added the flex class: https://github.com/rails/tailwindcss-rails/pull/71/files#diff-82d31adf98ad40bfa67ed05464d36790777703adead66a2478d97320061f136c

There's no explanation why it should be a flex container though.