feat(storiesNav): deep theming for stories nav panel #6098

mbellary-chwy · 2019-03-14T20:37:34Z

Feature:

deep theming for stories nav panel

What I did

deep theming for stories nav panel
updating README file with all the deep theme variables supported
updating LogoLink to support html elements

This is similar to the deep theming PR of SB 4.0:

ndelangen

I like the idea of making all this theme-able. I'm on the fence if we should add all these variables to ThemeVars though.

WDYT @shilman, @domyen ?

shilman · 2019-03-15T10:29:46Z

@ndelangen Yeah this seems way verbose to me. @MansoorBashaBellary what's the minimum you'd want to be able to style to achieve your desired output? I think we should add 1-2 vars at most.

codecov · 2019-03-15T11:19:31Z

Codecov Report

Merging #6098 into next will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             next    #6098      +/-   ##
==========================================
+ Coverage   37.11%   37.11%   +<.01%     
==========================================
  Files         648      648              
  Lines        9523     9524       +1     
  Branches     1352     1352              
==========================================
+ Hits         3534     3535       +1     
  Misses       5410     5410              
  Partials      579      579

Impacted Files	Coverage Δ
lib/ui/src/components/sidebar/SidebarSubheading.js	`100% <ø> (ø)`	⬆️
lib/theming/src/themes/light.ts	`100% <ø> (ø)`	⬆️
lib/theming/src/themes/dark.ts	`100% <ø> (ø)`	⬆️
lib/theming/src/base.ts	`100% <ø> (ø)`	⬆️
lib/ui/src/components/sidebar/SidebarItem.js	`96.15% <100%> (+0.15%)`	⬆️
lib/ui/src/components/sidebar/SidebarHeading.js	`88.46% <100%> (ø)`	⬆️
lib/ui/src/components/sidebar/Sidebar.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 997af33...90d641b. Read the comment docs.

codecov · 2019-03-15T11:19:31Z

Codecov Report

❗ No coverage uploaded for pull request base (next@97079c4). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             next    #6098   +/-   ##
=======================================
  Coverage        ?   37.11%           
=======================================
  Files           ?      648           
  Lines           ?     9524           
  Branches        ?     1352           
=======================================
  Hits            ?     3535           
  Misses          ?     5410           
  Partials        ?      579

Impacted Files	Coverage Δ
lib/ui/src/components/sidebar/SidebarSubheading.js	`100% <ø> (ø)`
lib/theming/src/themes/light.ts	`100% <ø> (ø)`
lib/theming/src/themes/dark.ts	`100% <ø> (ø)`
lib/theming/src/base.ts	`100% <ø> (ø)`
lib/ui/src/components/sidebar/SidebarItem.js	`96.15% <100%> (ø)`
lib/ui/src/components/sidebar/SidebarHeading.js	`88.46% <100%> (ø)`
lib/ui/src/components/sidebar/Sidebar.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97079c4...0ec9773. Read the comment docs.

mbellary-chwy · 2019-03-15T13:42:17Z

@ndelangen Yeah this seems way verbose to me. @MansoorBashaBellary what's the minimum you'd want to be able to style to achieve your desired output? I think we should add 1-2 vars at most.

@shilman - These are the minimum things that i could think off to get the desired output. What i can do instead is to wrap all the variables in storiesNav like:

storiesNav: {
  nav: {},
  navHeading: {},
  treeSubHeading: {},
  treeItemHeader: {},
  activeMenuLink: {},
  treeExpander: {},
  treeIcons: {}
}

But the only thing is in js files, i have to do theme.storiesNav.* to get the desired output. Please let me know what best thing to go ahead.

ndelangen · 2019-03-15T14:50:10Z

@MansoorBashaBellary I totally understand the request for more customization.

I was planning to enable this using addons.

I'm still unsure how much theming to allow.

On 1 hand I want users to be able to create the storybook of their dreams, but on the other hand, we don't want to have hundreds of theme variables, that we'll have to keep sane and compatible.

domyen · 2019-03-15T14:57:05Z

@ndelangen I think this adds more complexity and maintenance surface area for the UI code so I'd prefer to leave it mostly the same.

Storybook helps you build UIs, theming is an aesthetic choice that doesn't really contribute or detract from that value prop. I think we should keep the theming API as minimal as possible, that way maintainers can focus on building new functionality instead of maintaining support for theming edge cases into the future.

(Background: I worked on theming support with Norbert)

mbellary-chwy · 2019-03-15T16:14:50Z

@MansoorBashaBellary I totally understand the request for more customization.

I was planning to enable this using addons.

I'm still unsure how much theming to allow.

On 1 hand I want users to be able to create the storybook of their dreams, but on the other hand, we don't want to have hundreds of theme variables, that we'll have to keep sane and compatible.

Thanks @ndelangen and @domyen. We are maintaining our storybooks with stories nav panel themed from Storybooks 4.0. In order to use SB 5.0, we needed these changes.

I don't think these changes are going to make storybooks complex. But i totally agree that theme variables are to be minimal. Let me know if we can use any other approach that i am unaware of, to get this thing done.

adamlohner · 2019-03-17T15:52:56Z

Storybook helps you build UIs, theming is an aesthetic choice that doesn't really contribute or detract from that value prop. I think we should keep the theming API as minimal as possible, that way maintainers can focus on building new functionality instead of maintaining support for theming edge cases into the future.

If that's the long term stance on this topic (while not ideal, I'm not the one who has to maintain so it's an acceptable reason IMO), can additional attrs/classes/id's be added to the specific elements that this PR enables theming for so that we can still override them by injecting styles using the manager-head.html workaround?

domyen · 2019-03-17T16:02:29Z

@adamlohner good point. Perhaps adding classNames so folks could style via CSS is a reasonable middle ground here?

jonmilner · 2019-03-19T04:32:17Z

Not specifically related to the Storybook nav (I'm thread-jacking a bit here), but this seems relevant to theming within Storybook in general...

This has been one of my biggest pain points with open-source tools that use CSS-in-JS -- I certainly understand the appeal in terms of maintenance, but as an end-user, not having class names available in the DOM to make custom style overrides easy (or in some cases possible at all) can be incredibly frustrating. Even if it's not technically supported, simply making it possible to override default styling would go a long way -- and I'd happily accept the risk of minor upgrades breaking my custom styles.

Storybook helps you build UIs, theming is an aesthetic choice that doesn't really contribute or detract from that value prop. I think we should keep the theming API as minimal as possible, that way maintainers can focus on building new functionality instead of maintaining support for theming edge cases into the future.

Agree with @adamlohner -- while not ideal, it's certainly understandable. Storybook is an incredible tool as-is (the updates introduced in v5 are awesome). And taking theme customization too far could absolutely detract from the amazing work the Storybook team is doing.

I do, however, feel like it's not always simply about meaningless aesthetics, but rather the UX -- which is consistent with that value prop if an improved UX helps people build UIs.

Take the following example:

I want to help my team build UIs. Having the props documented in an intuitive, easy-to-read way will help them do that.

Unfortunately, it's hidden behind this little blue button:

I see there's an option to display it all on one screen, but it looks a bit clunky...

What I really want is for it to show up in the addons panel. I see that's not actually supported (#1147) but there's a workaround.

This is a lot closer to what I'd actually consider a good experience, but it could use some tweaking. addon-info seems to allow some custom styling:

But there's not much else I can do here (that I'm aware of, at least). And it's still a far cry from where I want it to be:

(^ These styles were achieved via editing styles in Chrome's dev tools. It doesn't seem this level of customization is actually possible.)

This is obviously just one example, but there have been more than a couple of areas that I've fought with only to realize overriding styles that would otherwise be very simple to do is either going to be way too much work, or is just not possible.

I think adding class names to the Storybook UI could be huge -- even just a few sprinkled throughout so that I could write selectors like #section .area h2 { ... } rather than something like #section > div:nth-child(2n) > div > div > div > h2 { ... }.

adamlohner · 2019-03-19T21:52:59Z

@jonmilner you could always write a custom addon to achieve the desired functionality rather than working around the existing notes implementation. I actually just did this and found the Addon API to be super flexible and easy to use.

domyen · 2019-04-05T18:06:50Z

Thanks everyone, I definitely appreciate the good thinking behind this PR. I'm closing this because there isn't clear consensus on theme API expansion and the impact on future maintainability.

@jonmilner That use case probably isn't in the scope of SB theming. Rather, it's the visual implementation and functionality of addons. I reckon you'll be pleased with the additional documentation features in Storybook Docs coming soon. Specifically the new docs addon that consolidates info/readme/notes addons into one.

If folks still want extra visual customization, I'd propose an alternate PR that assigns static classNames to DOM elements for styling.

- adding classNames to stories nav elements - updating LogoLink to support html elements This PR is a replacement for the deep theming PR: #6098

mbellary-chwy added feature request ui theming labels Mar 14, 2019

mbellary-chwy self-assigned this Mar 14, 2019

mbellary-chwy requested review from Hypnosphi, igor-dv, ndelangen, shilman and tmeasday as code owners March 14, 2019 20:37

mbellary-chwy force-pushed the feature/deep-theme-stories-nav-5.0 branch from b832752 to 90d641b Compare March 14, 2019 20:52

ndelangen reviewed Mar 15, 2019

View reviewed changes

Zarabadoo approved these changes Mar 15, 2019

View reviewed changes

feat(storiesNav): deep theming for stories nav panel

0ec9773

mbellary-chwy force-pushed the feature/deep-theme-stories-nav-5.0 branch from 90d641b to 0ec9773 Compare March 15, 2019 14:35

domyen closed this Apr 5, 2019

mbellary-chwy deleted the feature/deep-theme-stories-nav-5.0 branch April 20, 2019 18:20

mbellary-chwy added a commit that referenced this pull request Apr 20, 2019

feat(storiesNav): adding classNames to stories nav elements

9308908

- adding classNames to stories nav elements - updating LogoLink to support html elements This PR is a replacement for the deep theming PR: #6098

mbellary-chwy mentioned this pull request Apr 20, 2019

feat(storiesNav): adding classNames to stories nav elements #6571

Merged

mbellary-chwy added a commit that referenced this pull request Apr 21, 2019

feat(storiesNav): adding classNames to stories nav elements

88735c3

- adding classNames to stories nav elements - updating LogoLink to support html elements This PR is a replacement for the deep theming PR: #6098

mbellary-chwy added a commit that referenced this pull request Apr 21, 2019

feat(storiesNav): adding classNames to stories nav elements

f46a455

- adding classNames to stories nav elements - updating LogoLink to support html elements This PR is a replacement for the deep theming PR: #6098

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storiesNav): deep theming for stories nav panel #6098

feat(storiesNav): deep theming for stories nav panel #6098

mbellary-chwy commented Mar 14, 2019 •

edited

Loading

ndelangen left a comment

shilman commented Mar 15, 2019 •

edited

Loading

codecov bot commented Mar 15, 2019

codecov bot commented Mar 15, 2019 •

edited

Loading

mbellary-chwy commented Mar 15, 2019

ndelangen commented Mar 15, 2019

domyen commented Mar 15, 2019

mbellary-chwy commented Mar 15, 2019

adamlohner commented Mar 17, 2019 •

edited

Loading

domyen commented Mar 17, 2019

jonmilner commented Mar 19, 2019

adamlohner commented Mar 19, 2019

domyen commented Apr 5, 2019

feat(storiesNav): deep theming for stories nav panel #6098

feat(storiesNav): deep theming for stories nav panel #6098

Conversation

mbellary-chwy commented Mar 14, 2019 • edited Loading

What I did

ndelangen left a comment

Choose a reason for hiding this comment

shilman commented Mar 15, 2019 • edited Loading

codecov bot commented Mar 15, 2019

Codecov Report

codecov bot commented Mar 15, 2019 • edited Loading

Codecov Report

mbellary-chwy commented Mar 15, 2019

ndelangen commented Mar 15, 2019

domyen commented Mar 15, 2019

mbellary-chwy commented Mar 15, 2019

adamlohner commented Mar 17, 2019 • edited Loading

domyen commented Mar 17, 2019

jonmilner commented Mar 19, 2019

adamlohner commented Mar 19, 2019

domyen commented Apr 5, 2019

mbellary-chwy commented Mar 14, 2019 •

edited

Loading

shilman commented Mar 15, 2019 •

edited

Loading

codecov bot commented Mar 15, 2019 •

edited

Loading

adamlohner commented Mar 17, 2019 •

edited

Loading