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

chore(devenv): re-introduce theme switcher #3073

Merged

Conversation

@asudoh
Copy link
Member

commented Jun 14, 2019

This change (re-)introduces a theme switcher at the top right in dev env. It's done by creating a dev env Sass build option (--use-custom-properties) that emits style ruleset like:

.bx--foo {
  color: #171717; // $text-01, escape hatch for IE11
  color: var(--text-01);
}

This Sass build option is:

  • Turned off by default for regular dev env, because it doubles Sass build time
  • Turned on by default for deployed dev env so our designers can fiddle with different themes there

Refs #3046.

Changelog

New

  • --use-custom-properties dev env Sass build option (see above).

Changed

  • Upgrades carbon-components-react used in dev env to v10.

Testing / Reviewing

Testing should make sure our dev env is not broken.

@project-bot project-bot bot added this to PR: needs review 🕵️ in Carbon Support Jun 14, 2019

@netlify

This comment has been minimized.

Copy link

commented Jun 14, 2019

Deploy preview for carbon-elements ready!

Built with commit 1014db7

https://deploy-preview-3073--carbon-elements.netlify.com

@netlify

This comment has been minimized.

Copy link

commented Jun 14, 2019

Deploy preview for the-carbon-components ready!

Built with commit 1014db7

https://deploy-preview-3073--the-carbon-components.netlify.com

@netlify

This comment has been minimized.

Copy link

commented Jun 14, 2019

Deploy preview for carbon-components-react failed.

Built with commit 1014db7

https://app.netlify.com/sites/carbon-components-react/deploys/5d08f58421d3b1000d74d28f

chore(devenv): re-introduce theme switcher
This change (re-)introduces a theme switcher at the top right in dev
env. It's done by creating a dev env Sass build option
(`--use-custom-properties`) that emits style ruleset like:

```scss
.bx--foo {
  color: #171717; // $text-01, escape hatch for IE11
  color: var(--text-01);
}
```

This Sass build option is:

* Turned **off** by default for regular dev env, because it doubles
  Sass build time
* Turned **on** by default for deployed dev env so our designers can
  fiddle with different themes there

Refs #3046.

@asudoh asudoh force-pushed the asudoh:theme-switcher-custom-properties branch from df9db05 to 7027bb7 Jun 14, 2019

@asudoh

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

@shixiedesign The demo can be seen at https://deploy-preview-3073--the-carbon-components.netlify.com/?nav=accordion. Ability to change individual theme tokens haven't been implemented yet, will address in a separate PR. Please let me know if you'd like kitchen-sink style UI seen in http://themes.carbondesignsystem.com - I'll add one if so. Thanks!

@asudoh asudoh marked this pull request as ready for review Jun 14, 2019

@asudoh asudoh requested a review from shixiedesign Jun 14, 2019

@joshblack

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

Should we wait for #3033 to resolve to make sure the strategies are aligned?

@asudoh

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

How to emit CSS custom properties is not the core of the change; Can change that any approach as long as it's flexible enough that fits in the purpose of the theme switcher introduced in this PR.

@emyarod
Copy link
Member

left a comment

it looks like the legacy dropdown is being imported for the theme switcher

image

vs

image

@joshblack

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@asudoh not sure I understand, just felt like a lot of stuff was being added that felt coupled to that strategy. Let me know how you're thinking this will felt together if you get the chance 👍

@asudoh

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

@joshblack The theme switcher is implemented by existing Sass maps in @carbon/themes - No substantial thing is added to implement that - Not a lot of stuffs like you said. All of changes are dev-env only thing. When we determine the future theming strategy, we can simply change the theme switcher code.

@asudoh

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

@emyarod You are right, due to build problem I needed to put V9 dropdown there - But it's dev env-only thing.

@shixiedesign
Copy link
Contributor

left a comment

I am sooooo happy to see the themes being rendered with a simple dropdown. Can't wait to start verifying each component's styling in each theme. Only 1 error I noticed, hopefully easy fixes:

Looks like the background in component live example is pulling the incorrect token. Should not be using $ui-02, please use $ui-background instead.
image

Carbon Support automation moved this from PR: needs review 🕵️ to PR: review in progress 🗣️ Jun 18, 2019

@shixiedesign

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@joshblack right this is only for internal use. Design desperately needed this functionality to verify the correctness of our token values and token assignment to components. I have no idea what "emit custom properties" is haha – this PR does not define / restrict the way Carbon is going to ship themeing function to our users.

@shixiedesign
Copy link
Contributor

left a comment

Perfect! This will help me so much. Thanks! 🙌

@emyarod
Copy link
Member

left a comment

looks good to me!

Carbon Support automation moved this from PR: review in progress 🗣️ to PR: ready to merge 🔜 Jun 18, 2019

@emyarod
Copy link
Member

left a comment

actually just one thing I noticed, after navigating to a new page, the selection in the theme switcher dropdown resets but the theme itself does not change

@asudoh

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

Good catch @emyarod! Fixed.

@asudoh asudoh merged commit b1f3a52 into carbon-design-system:master Jun 18, 2019

2 of 4 checks passed

netlify/carbon-components-react/deploy-preview Deploy preview processing.
Details
netlify/carbon-elements/deploy-preview Deploy preview processing.
Details
ci/circleci: system Your tests passed on CircleCI!
Details
netlify/the-carbon-components/deploy-preview Deploy preview ready!
Details

Carbon Support automation moved this from PR: ready to merge 🔜 to Issue/PR: Closed 🙌 Jun 18, 2019

@asudoh asudoh deleted the asudoh:theme-switcher-custom-properties branch Jun 18, 2019

@joshblack

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@asudoh why are we merging things when there are open questions on the PRs? This is adding more stuff to our dev environment for this use-case that may or may not make sense depending on our theming strategy (for example, adding a feature flag for custom properties and toggling our dev build pipeline as a result). We really should not be merging things in if there are open questions or if someone is asking for clarification.

@joshblack

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@shixiedesign yes, I totally get that. This is a comment towards implementation and how that relates to the dev env and our strategy towards that 👍

emyarod added a commit to emyarod/carbon that referenced this pull request Jun 18, 2019

chore(devenv): re-introduce theme switcher (carbon-design-system#3073)
This change (re-)introduces a theme switcher at the top right in dev
env. It's done by creating a dev env Sass build option
(`--use-custom-properties`) that emits style ruleset like:

```scss
.bx--foo {
  color: #171717; // $text-01, escape hatch for IE11
  color: var(--text-01);
}
```

This Sass build option is:

* Turned **off** by default for regular dev env, because it doubles
  Sass build time
* Turned **on** by default for deployed dev env so our designers can
  fiddle with different themes there

Refs carbon-design-system#3046.
@asudoh

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

@joshblack This PR is independent from our theming strategy.

@joshblack

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

@asudoh that makes sense, just was hoping we could chat about things before items get merged in. One thing I wanted to raise is that we should look to add this into our React environment first instead of our vanilla environment.

cal-smith added a commit to cal-smith/carbon-components that referenced this pull request Jun 21, 2019

chore(devenv): re-introduce theme switcher (carbon-design-system#3073)
This change (re-)introduces a theme switcher at the top right in dev
env. It's done by creating a dev env Sass build option
(`--use-custom-properties`) that emits style ruleset like:

```scss
.bx--foo {
  color: #171717; // $text-01, escape hatch for IE11
  color: var(--text-01);
}
```

This Sass build option is:

* Turned **off** by default for regular dev env, because it doubles
  Sass build time
* Turned **on** by default for deployed dev env so our designers can
  fiddle with different themes there

Refs carbon-design-system#3046.

cal-smith added a commit to cal-smith/carbon-components that referenced this pull request Jun 21, 2019

chore(devenv): re-introduce theme switcher (carbon-design-system#3073)
This change (re-)introduces a theme switcher at the top right in dev
env. It's done by creating a dev env Sass build option
(`--use-custom-properties`) that emits style ruleset like:

```scss
.bx--foo {
  color: #171717; // $text-01, escape hatch for IE11
  color: var(--text-01);
}
```

This Sass build option is:

* Turned **off** by default for regular dev env, because it doubles
  Sass build time
* Turned **on** by default for deployed dev env so our designers can
  fiddle with different themes there

Refs carbon-design-system#3046.

cal-smith added a commit to cal-smith/carbon-components that referenced this pull request Jun 21, 2019

chore(devenv): re-introduce theme switcher (carbon-design-system#3073)
This change (re-)introduces a theme switcher at the top right in dev
env. It's done by creating a dev env Sass build option
(`--use-custom-properties`) that emits style ruleset like:

```scss
.bx--foo {
  color: #171717; // $text-01, escape hatch for IE11
  color: var(--text-01);
}
```

This Sass build option is:

* Turned **off** by default for regular dev env, because it doubles
  Sass build time
* Turned **on** by default for deployed dev env so our designers can
  fiddle with different themes there

Refs carbon-design-system#3046.
@mattrosno mattrosno referenced this pull request Jul 12, 2019
24 of 41 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.