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

Support for inline theming? #1648

Open
teddyward opened this issue Feb 1, 2023 · 8 comments
Open

Support for inline theming? #1648

teddyward opened this issue Feb 1, 2023 · 8 comments
Labels
enhancement New feature or request
Milestone

Comments

@teddyward
Copy link

teddyward commented Feb 1, 2023

The Carbon design spec has recommendations about using an inline theme to essentially apply a different theme to one part of the app vs. the global theme. The storybook for React implements that kind of behavior here. All the theme examples in Svelte, however, only apply a single theme, and from my own testing, I don't believe that it is possible to do the following in Svelte:

<Theme theme="g10">
      <p>g10 theme</p>
</Theme>
<Theme theme="white">
      <p>white theme</p>
</Theme>

Before making any proposals, I'm seeking to verify:

  • If it is true that Inline Theming isn't currently supported by this library
  • If it is true that Inline Theming isn't supported, whether or not that is an intentional design decision
@theetrain
Copy link
Collaborator

theetrain commented Feb 2, 2023

Although not explicitly mentioned in the v1 Roadmap, we intend to support inline theming in some way once Carbon v11 styles are adopted. At this time, carbon-components-svelte ships with Carbon v10 styles and themes.

Since Carbon v11 styles come with CSS variables, it should be possible to support theming using a context wrapper like in the example you shared. Implementation details still need to be determined, such as use of style props or injecting a reusable themed stylesheet for progressive enhancement.

Related:

@theetrain theetrain added the enhancement New feature or request label Feb 2, 2023
@theetrain theetrain added this to the v1.0.0 milestone Feb 2, 2023
@theetrain theetrain mentioned this issue Feb 20, 2023
5 tasks
@gregorw
Copy link
Contributor

gregorw commented Mar 20, 2023

What this means is that we would change the current Theme component which I think is the right thing to do. How do we phase out the old component? Deprecate or rename it?

@theetrain
Copy link
Collaborator

@gregorw my current plan is to provide a default slot for Theme to allow inline theming. I'm still figuring out how to lazy-load component CSS variables so it's still a work in progress:

{#if $$slots.default}
<div data-carbon-theme="{theme}">
<slot theme="{theme}" />
</div>
{/if}

See branch v11-theme.

@gregorw
Copy link
Contributor

gregorw commented Mar 22, 2023

Hm… not sure I would mix the legacy behavior with the new implementation. Also, how can we merge this to main/master when master still doesn’t have v11 styles?

@theetrain
Copy link
Collaborator

We won't merge this into main as-is, there's a feature branch for v11 styles and once every component has been updated to v11, then it will be part of the main package. Tracked in #1636

By legacy behaviour are you referring to v10 styles, or something else?

@gregorw
Copy link
Contributor

gregorw commented Mar 23, 2023

We won't merge this into main as-is, there's a feature branch for v11 styles and once every component has been updated to v11, then it will be part of the main package. Tracked in #1636

My concern is that this will be a very long-lived branch, which is not good. I’d much rather try to merge smaller changes onto the main branch. Would it be possible to do these steps separately and merge them into main?

Those are all separate steps. Merging smaller patches at once would give us more progress on the main branch quicker with less risk.

By legacy behaviour are you referring to v10 styles, or something else?

What I mean with “legacy behavior” is that the current Theme component (Svelte specific) changes the theme for the entire page. The idea of the new CDS v11 Theme component is to provide inline theming capabilities. The React (standard) implementation also provides a GlobalTheme component, though.

What I am saying is that we can rename Theme to GlobalTheme and merge that into main. This leaves space for a v11 Theme component that handles inline / local theming using v11 styles. I wouldn’t blend those two components into one.

gregorw added a commit to gregorw/carbon-components-svelte that referenced this issue Mar 24, 2023
Carbon Design System provides the idea of [inline theming](https://carbondesignsystem.com/guidelines/color/implementation/#how-inline-theming-works). As was mentioned in carbon-design-system#1648 the Carbon standard implementation is [documented here](https://react.carbondesignsystem.com/?path=/docs/components-theme--playground). It says:

> The `GlobalTheme` and `Theme` components allow you to specify the theme for a page, or for a part of a page, respectively. `Theme` is most often used to implement inline theming where you can style a portion of your page with a particular theme.

What this means for `carbon-components-svelte` is that we should rename the existing `Theme` component to `GlobalTheme`. This leads us a tiny bit closer to [feature parity with Carbon v11](carbon-design-system#1614) and gives room for a new component dedicated to v11 inline theming.
gregorw added a commit to gregorw/carbon-components-svelte that referenced this issue Mar 25, 2023
Carbon Design System provides the idea of [inline theming](https://carbondesignsystem.com/guidelines/color/implementation/#how-inline-theming-works). As was mentioned in carbon-design-system#1648 the Carbon standard implementation is [documented here](https://react.carbondesignsystem.com/?path=/docs/components-theme--playground). It says:

> The `GlobalTheme` and `Theme` components allow you to specify the theme for a page, or for a part of a page, respectively. `Theme` is most often used to implement inline theming where you can style a portion of your page with a particular theme.

What this means for `carbon-components-svelte` is that we should rename the existing `Theme` component to `GlobalTheme`. This leads us a tiny bit closer to [feature parity with Carbon v11](carbon-design-system#1614) and gives room for a new component dedicated to v11 inline theming.
@theetrain
Copy link
Collaborator

theetrain commented Mar 25, 2023

My concern is that this will be a very long-lived branch, which is not good. I’d much rather try to merge smaller changes onto the main branch. Would it be possible to do these steps separately and merge them into main?

I appreciate your insight. I think phasing in v11 components one at a time to master is a good idea; I realize having a waterfall approach to v11 adoption may be tougher on us than anticipated.

Here's how that can look:

  1. We set up svelte-package on master asap; and move components from src/carbon-components-svelte back to src to not break deep imports for now, i.e. import Button from 'carbon-components-svelte/src/Button'
  2. Set up GlobalTheme (thanks for chore: rename Theme to GlobalTheme #1702, we'll have a look)
  3. Push v11 <Button> to master
  4. Reach out to contributors, and then continue with remaining components

During this interim phase, the consumption side will look like this:

  • Some v10 components, some v11
  • v11 components will include their styles. For example, Button will have import './Button.css' in its code, which is pre-built from SCSS. The decision to use import './Button.css' over <style>@use "@carbon/styles/scss/../Button";</style> isn't final, but is likely due to the constraint of Carbon SCSS including ButtonSet styles all within Button.scss (other components are structured similarly), and I figured it would lead to smaller bundles if we use JS import.
  • Trade-off: The v10 components are styled with SCSS or the provided all.css, white.css, etc while v11 components provide their own styles. This will result in larger bundles as we phase in v11 styles since there may be duplicate CSS such as v10 Button and v11 Button styles.
  • v11 components can be customized with --style-props or CSS variables

Let me know if that sounds good, I'd like to share this plan in #1614.

gregorw added a commit to gregorw/carbon-components-svelte that referenced this issue Mar 26, 2023
Carbon Design System provides the idea of [inline theming](https://carbondesignsystem.com/guidelines/color/implementation/#how-inline-theming-works). As was mentioned in carbon-design-system#1648 the Carbon standard implementation is [documented here](https://react.carbondesignsystem.com/?path=/docs/components-theme--playground). It says:

> The `GlobalTheme` and `Theme` components allow you to specify the theme for a page, or for a part of a page, respectively. `Theme` is most often used to implement inline theming where you can style a portion of your page with a particular theme.

What this means for `carbon-components-svelte` is that we should rename the existing `Theme` component to `GlobalTheme`. This leads us a tiny bit closer to [feature parity with Carbon v11](carbon-design-system#1614) and gives room for a new component dedicated to v11 inline theming.
@gregorw
Copy link
Contributor

gregorw commented Mar 26, 2023

I think phasing in v11 components one at a time to master is a good idea;

That’s not exactly what I meant.

  1. We set up svelte-package on master asap; and move components from src/carbon-components-svelte back to src to not break deep imports for now, i.e. import Button from 'carbon-components-svelte/src/Button'

I’m not sure svelte-package is a requirement for v11 style adoption. What I meant with my previous comment was that we can adapt all components to v11 in a single PR, but without changing anything else (build, class prefixes, etc.). In fact, CDS v11 has limited changes that I think we can handle okay.

  1. Set up GlobalTheme (thanks for chore: rename Theme to GlobalTheme #1702, we'll have a look)

👍

  1. Push v11 <Button> to master

v11 changes for buttons are quite moderate: 15f2875. I wouldn’t change class prefixes at the same time as adopting to the new styles as in 6fc0ab3. This is not necessary and it leads to noise that makes reviewing the actual meaningful changes more difficult.

  • Some v10 components, some v11
  • v11 components will include their styles. For example, Button will have import './Button.css' in its code, which is pre-built from SCSS. The decision to use import './Button.css' over <style>@use "@carbon/styles/scss/../Button";</style> isn't final, but is likely due to the constraint of Carbon SCSS including ButtonSet styles all within Button.scss (other components are structured similarly), and I figured it would lead to smaller bundles if we use JS import.

Maybe we can draft a PR and discuss this there?

  • v11 components can be customized with --style-props or CSS variables

This is a discussion worthwhile having. But, I would keep it separate from v11 styles. This can come later. I think what we need is minimal changes that lead to v11 on master the quickest.

Let me know if that sounds good, I'd like to share this plan in #1614.

Before we share this plan maybe we can draft a PR that demonstrates feasibility of your idea. In the meantime I would like to try an alternative way that imposes minimal changes on the current setup: #1709.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants