qwik icon indicating copy to clipboard operation
qwik copied to clipboard

feat(qwik-core): `useStylesWithScope$` that introduces `:scope` pseudo class to scope styles to the component manually.

Open genki opened this issue 1 year ago • 4 comments

Overview

What is it?

  • [x] Feature / enhancement
  • [ ] Bug
  • [ ] Docs / tests / types / typos

Description

The useStyles$ can style entire page but it couldn't scope to components. The useStylesScoped$ can style scoped to the component but it couldn't style its child components. So I added one more thing, the useStyleWithScope$ that introduces the :scope pseudo class to scope styles manually.

By using this function, you can use :scope pseudo class to be replaced with .${scopeId}.

:scope {
  background: red
}

The above css is to be like this:

.⭐️5yhoz-0 {
  background: red
}

Use cases and why

While the useStylesScoped$ put the scopeId to every rules in the CSS, this function can select rules to put it. So you can scope styles to the component and its children such as <Link> components too.

Checklist:

  • [x] My code follows the developer guidelines of this project
  • [x] I have performed a self-review of my own code
  • [x] I have made corresponding changes to the documentation
  • [ ] Added new tests to cover the fix / functionality

genki avatar Jun 28 '24 03:06 genki

Deploy request for qwik-insights rejected.

Name Link
Latest commit 23492ca35e723f738c276084d5ec4f05ee8d3859

netlify[bot] avatar Jun 28 '24 03:06 netlify[bot]

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

commit: 12f6a55

@builder.io/qwik

npm i https://pkg.pr.new/@builder.io/qwik@6614
@builder.io/qwik-city

npm i https://pkg.pr.new/@builder.io/qwik-city@6614
eslint-plugin-qwik

npm i https://pkg.pr.new/eslint-plugin-qwik@6614
create-qwik

npm i https://pkg.pr.new/create-qwik@6614

templates

pkg-pr-new[bot] avatar Jun 28 '24 03:06 pkg-pr-new[bot]

Thanks @genki we used to have a team discussion for new APIs so I marked this PR with the correct label. 😊

gioboa avatar Jun 28 '24 11:06 gioboa

@gioboa Thanks :)

genki avatar Jun 28 '24 15:06 genki

I'm sorry because the core team didn't have the possibility to talk about this PR yet. I'm thinking in advance for a possible question. Are there any alternatives at this new API? Can we achieve a similar solution with the current codebase?

gioboa avatar Jul 04 '24 20:07 gioboa

@gioboa I think there's noways to do this using existing public APIs. This is depending on the scopeId that is made of the styleId and iCtx.$i$. There is only one way to know the scopeId, that is calling useStylesScoped$ and get its return value. But the call of that modifies the style string with the scope entirely and difficult to unscope them. In addition, there's no exposed way to add the scopeId to the invoke context other than the useStylesScoped$. I think there is another choose to accomplish this function that is to have the useStyles$ accept 2 extra arguments to pass them to the _useStyles.

genki avatar Jul 04 '24 21:07 genki

I see. Thanks

gioboa avatar Jul 04 '24 21:07 gioboa

  • can you add an example, and why you can't just add the scopeId class on all the children you want to style
  • can you put the documentation into readme.md like it says in the comments you copied
  • can you run pnpm api.update
  • could this be achieved instead via an option object on useStyles$?
  • replacing all :scope seems a little bit brittle, but it's probably ok

wmertens avatar Jul 04 '24 21:07 wmertens

@wmertens

can you add an example, and why you can't just add the scopeId class on all the children you want to style

The most usual example is styling of the <Link> component as a children. There can be components that have no their own fixed style but their looks want to be depending on the context of where it is, such as <Link> component. In such cases, this function can control styles of these components by their parent's scope. To add the scopeId to all the children, there are two drawbacks.

  1. the parent of parent or the ancestors can't provide its styling context.
  2. if the child component have DOM structure inside, it is need to put the scopeId to all of elements in order to respect the parent scopeId. This is messy.

can you put the documentation into readme.md like it says in the comments you copied

I am not familiar with way of editing the documentation at this repo. But I will try.

can you run pnpm api.update

Yes.

could this be achieved instead via an option object on useStyles$?

Yes, the heart of this PR is just using underlying _useStyles. If its 3 arguments are fully exposed as option object of the useStyles$, there is no need to add this new API.

genki avatar Jul 04 '24 21:07 genki

I have rerun the api.update

genki avatar Jul 04 '24 22:07 genki

So for the readme, look at the ../readme.md file, see how it's documented there. There's a docSync command (I think) that copies the readme parts to the jsdoc.

I'm still not entirely convinced about the utily. Can you provide a real example?

And actually, why not always support :scope? Maybe call it :q-scope though so it never gets mixed up with future css syntax

wmertens avatar Jul 05 '24 08:07 wmertens

@wmertens For the real example, I am using the SASS for the css processor, so it is able to put the :scope pseudo class to the top level selector of the sass file like this:

form:scope
  width: 100%
  input[type=text]
    border: 1px solid black
    color: black
  // ...

This sass can provide the scoped style for the component (the form component at this example) itself, children and ascendants.

Suppose the form component is like this:

import TextField from "~/src/components/form/text";
import styles from "./styles.sass?inline";
export default component$(() => {
  useStylesWithScope$(styles);
  // ...
  return <form onSubmit={...}>
    <TextField name="foo" />
  </from>
});

As of the TextField has independent style if I use the useStylesScoped$ and I have to pass the scopeId, and put it at the every elements in that component that want to inherit the parent's style. If the TextField has its own child components, I have to pass the scopeId to them too recursively. If I use the useStylesWithScope$, I have to do nothing except to put the :scope once.

So this function can provide easily switchable styling context for styling agnostic UI components such as <Link>, <Filed>, <Calendar> and so on. Passing the scopeId to every ascendants is very hard if there are many such components.

I don't have strong preference about the name of the pseudo class :scope. I just chosen as it was looking like the real pseudo class :) I think the :q-scope is nicer.

genki avatar Jul 05 '24 10:07 genki

So this only works if there was a scoped style above it?

I would prefer that it is part of useStyles$, so no real extra API, just extra capabilities.

It also needs tests.

wmertens avatar Jul 09 '24 15:07 wmertens

@wmertens Yes. As of the :q-scope will be replaced with the scopeId, the dom elements must have the scopeId on its classList to be scoped by the component. That is controlled by the 3rd argument of the _useStyles. At any rate, using the useStyles$ instead of the new API to do that, the :q-scope is just a user defined transform function that is maintained by application developers. I will try to change the useStyles$ to have the option object to do it.

genki avatar Jul 09 '24 17:07 genki

I have grep the source but couldn't found the tests for the useStyles$ and useStylesScoped$. Is there something about the test that I can use as references?

genki avatar Jul 09 '24 18:07 genki

Is there something about the test that I can use as references?

In the e2e tests you can add a component in the starter (https://github.com/QwikDev/qwik/blob/e05e3546db360a62ff61eb905daa48bfdce3f91e/starters/apps/e2e/src/components/styles/styles.tsx) and then add a test to https://github.com/QwikDev/qwik/blob/e05e3546db360a62ff61eb905daa48bfdce3f91e/starters/e2e/e2e.style.spec.ts

and then you can tests with pnpm test.e2e.chromium

wmertens avatar Jul 09 '24 18:07 wmertens

Thanks a lot for your contribution @genki !

We're about to add a proper RFC process soon and I think this could be a good candidate for discussion We'll schedule a dedicated meeting to go over it and evaluate the pros / cons and we'll invite you to be part of the process.

This should be done in the next few weeks (hopefully in the next 2) We'll keep you posted (and I'll also DM you)

Thanks again!

shairez avatar Jul 09 '24 20:07 shairez

@wmertens Thank you for information. I added a test by referencing it.

@shairez All right.

genki avatar Jul 09 '24 21:07 genki

Hey @genki Good news! we've added the RFC process Would you mind open up a proposal for this on https://github.com/QwikDev/qwik-evolution/discussions ?

Thanks!

shairez avatar Sep 17 '24 19:09 shairez

@shairez Thank you for information :)

genki avatar Sep 22 '24 00:09 genki

Closing this for now. Will re-open after an RFC is created for this

thejackshelton avatar Oct 01 '24 18:10 thejackshelton