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

UI: Fix theming of elements inside bars #26527

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Mar 15, 2024

Closes #26448

What I did

In Storybook 8.0, we have broken theming for elements inside bars. The theming variables "barTextColor", "barHoverColor" and "barSelectedColor" were mostly ignored and mainly other variable colors were applied.

This commit mainly ensures, that the default dark and light theming doesn't change. If "barTextColor", "barHoverColor" or "barSelectedColor" are set by the user, they get normally applied to the icon and text elements inside the bar areas, like the topbar, bottom-bar or the tab area in the addons panel.

Additionally, I have introduced a hover effect on the Tabs inside the TabsPanel. Until now, a hover didn't change the color at all.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  1. yarn task -> Compile all packages
  2. Go to code/ui/.storybook and create a new file MyTheme.js and put the following content in:
import { create } from '@storybook/theming/create';

export default create({
  base: 'light',

  // Toolbar default and active colors
  barTextColor: 'violet',
  barSelectedColor: 'yellow',
  barHoverColor: 'green',
  barBg: 'red',
});
  1. Import the theme in code/ui/.storybook/manager.tsx:
import React from 'react';
import { addons, types } from '@storybook/manager-api';
import startCase from 'lodash/startCase.js';
+ import MyTheme from './MyTheme.js';

addons.setConfig({
  sidebar: {
    renderLabel: ({ name, type }) => (type === 'story' ? name : startCase(name)),
  },
+  theme: MyTheme,
});
  1. Make sure that the bar styles are properly applied in the TopBar:
Bildschirmfoto 2024-03-15 um 21 35 32
  1. Make sure that the bar styles are properly applied in the Addon Panel:
Bildschirmfoto 2024-03-15 um 21 32 44
  1. Make sure the bottom bar in mobile mode is properly styled:
Bildschirmfoto 2024-03-15 um 21 34 09
  1. Adjust the theming so that it applies to dark mode, and repeat the testing instructions.
  2. Remove the custom theming and ensure the default dark and light modes haven't changed.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-26527-sha-db4e08ae. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information
Published version 0.0.0-pr-26527-sha-db4e08ae
Triggered by @valentinpalkovic
Repository storybookjs/storybook
Branch valentin/fix-theming-for-bar-areas
Commit db4e08ae
Datetime Fri Mar 15 20:41:38 UTC 2024 (1710535298)
Workflow run 8302078089

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=26527

In Storybook 8.0, we have broken theming for elements inside bars.
The theming variables "barTextColor", "barHoverColor" and "barSelectedColor" were mostly
ignored and mainly other variable colors were applied.

This commit mainly ensures, that the default dark and light theming doesn't change.
If "barTextColor", "barHoverColor" or "barSelectedColor" are set by the user,
they get normally applied to the icon and text elements inside the bar areas,
like the topbar, bottom-bar or the tab area in the addons panel.
@valentinpalkovic valentinpalkovic changed the title Fix theming of elements inside bars UI: Fix theming of elements inside bars Mar 15, 2024
@valentinpalkovic valentinpalkovic self-assigned this Mar 15, 2024
@valentinpalkovic valentinpalkovic added ci:daily Run the CI jobs that normally run in the daily job. bug ui labels Mar 15, 2024
Copy link

nx-cloud bot commented Mar 15, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a76f857. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@valentinpalkovic
Copy link
Contributor Author

All reported visual changes in Chromatic are unrelated to this PR.

@valentinpalkovic valentinpalkovic marked this pull request as ready for review March 15, 2024 21:10
Copy link
Contributor

@cdedreuille cdedreuille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @valentinpalkovic for finding a nice clean work around here. I don't really like to add some custom code in the Button itself but I'm not sure we have that much choice.

Perhaps you could add a comment to explain why we had to do that and the fact that we will remove it in Theming 2.0

@valentinpalkovic valentinpalkovic added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Mar 18, 2024
@valentinpalkovic valentinpalkovic merged commit 3e0de54 into next Mar 18, 2024
95 of 107 checks passed
@valentinpalkovic valentinpalkovic deleted the valentin/fix-theming-for-bar-areas branch March 18, 2024 11:00
@github-actions github-actions bot mentioned this pull request Mar 18, 2024
15 tasks
storybook-bot pushed a commit that referenced this pull request Mar 19, 2024
…ar-areas

UI: Fix theming of elements inside bars
(cherry picked from commit 3e0de54)
@github-actions github-actions bot mentioned this pull request Mar 19, 2024
11 tasks
storybook-bot pushed a commit that referenced this pull request Mar 19, 2024
…ar-areas

UI: Fix theming of elements inside bars
(cherry picked from commit 3e0de54)
storybook-bot pushed a commit that referenced this pull request Mar 19, 2024
…ar-areas

UI: Fix theming of elements inside bars
(cherry picked from commit 3e0de54)
storybook-bot pushed a commit that referenced this pull request Mar 19, 2024
…ar-areas

UI: Fix theming of elements inside bars
(cherry picked from commit 3e0de54)
storybook-bot pushed a commit that referenced this pull request Mar 19, 2024
…ar-areas

UI: Fix theming of elements inside bars
(cherry picked from commit 3e0de54)
storybook-bot pushed a commit that referenced this pull request Mar 19, 2024
…ar-areas

UI: Fix theming of elements inside bars
(cherry picked from commit 3e0de54)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:daily Run the CI jobs that normally run in the daily job. patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Theming barHoverColor property not working in v8
2 participants