Conversation
SteffenDE
force-pushed
the
sd-colocated
branch
from
563e917 to
316c77a
Compare
SteffenDE
force-pushed
the
sd-colocated-scoper
branch
from
9f20d9d to
d5f2b1f
Compare
green-david
left a comment
green-david
left a comment
There was a problem hiding this comment.
@SteffenDE Overall, this looks really great. Very much appreciate all your work here. I left a few general thoughts/comments/questions for you to ponder 👍
| config :phoenix_live_view, :root_tag_attribute, "phx-r" | ||
|
|
||
| config :phoenix_live_view, Phoenix.LiveView.ColocatedCSS, | ||
| scoper: Phoenix.LiveViewTest.Support.CSSScoper |
There was a problem hiding this comment.
Just curious if you think we should make this as specific as scoper or if we should make it more generic like transformer?
There was a problem hiding this comment.
Great question! Indeed you could do anything there, but I don't know if there's a practical use case despite scoping?
| The `directives` needs to be a keyword list that supports the following options: | ||
|
|
||
| * `root_tag_attribute`: A `{key, value}` tuple that will be added a | ||
| an attribute to all "root tags" of the template defining the scoped CSS tag. | ||
| See the section on root tags below for more information. | ||
| * `tag_attribute`: A `{key, value}` tuple that will be added as an attribute to | ||
| all HTML tags in the template defining the scoped CSS tag. |
There was a problem hiding this comment.
Just double-checking, this is here due to the fact that MacroComponent is still private api, yes?
| By default, Colocated CSS is not scoped. This means that the styles defined in a Colocated CSS block are extracted as is. | ||
| However, LiveView supports scoping Colocated CSS by defining a `:scoper` module implementing the `Phoenix.LiveView.ColocatedCSS.Scoper` | ||
| behaviour. When a `:scoper` is configured, Colocated CSS that is not defined with the `global` attribute will be scoped | ||
| according to the configured scoper. |
There was a problem hiding this comment.
I think it could be somewhat confusing that even if your ColocatedCSS block doesn't have global defined, if you don't have a scoper configured, it is essentially global.
Should we perhaps warn if we encounter a non-global ColocatedCSS block without a scoper defined?
I also am curious if you considered something like scoper={YourModuleImplementingBehaviourHere} directly on each ColocatedCSS block rather than a global app-config level scoper. That way you could just get rid of the global attribute entirely, as all ColocatedCSS would be global unless a scoper is explicitly provided.
If we do go this route (which I don't have any problems with), I think it may make sense to flip this so you don't have to specify global but you instead have to specify scoped on the ColocatedCSS tag to indicate that you want it to be scoped perhaps 🤔
There was a problem hiding this comment.
We should probably warn, yeah. The reason why I wouldn't go the scoper={} route is that you most likely want to use a scoper most of the time and since macro components work the way they do you cannot really have an abstraction that applies the scoper={MyScoper} for you without writing it in each template. We could allow scoper to override the default scoper though.
For flipping it it's a similar argument: if we expect most people to want scoped by default, needing to write it each time may be bad UX? Happy to change my mind here though, I agree that scoped and scoped={...} do feel more consistent.
There was a problem hiding this comment.
The more I think about it, the more I am on-board with this approach and a warning to prevent shooting yourself in the foot. Scoped-by-default should be encouraged, so being a bit noisy to make sure you aren't accidentally creating global CSS seems prudent. Forcing you to tag each ColocatedCSS block with global even if you don't have a scoper defined seems fine 👍
2 participants
See #4138.
Example scoped setup using PostCSS: SteffenDE/phx_colocated_postcss_example@e2cef37