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

Fix #4829: render non-grouped knobs in the ALL tab #5106

Merged
merged 2 commits into from
Jan 4, 2019

Conversation

Jessidhia
Copy link
Contributor

Issue: #4829

What I did

When there are knobs both with and without a groupId, the knobs without a groupId were not getting rendered at all.

I made them be rendered as part of the content of the (normally contentless) ALL tab.

How to test

  • Is this testable with Jest or Chromatic screenshots?
    • I don't know about screenshots but I wrote Jest tests.
  • Does this need an update to the documentation?
    • No, this fixes something that is documented to work but doesn't

PS: Oops, I only saw the notice about which branch to use as base after I wrote this. I need this to be in master FWIW. What do?


const titles = wrapper
.find(TabsState)
// TabsState will replace the <div/> that Panel actually makes with a <Tab/>
Copy link
Contributor Author

@Jessidhia Jessidhia Dec 27, 2018

Choose a reason for hiding this comment

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

The way TabsState works is, tbh, scary 😓

It's not even possible to refactor the objects with render / title into a component (which would be way cleaner) because that'd break TabsState looking into its children.

I assume it's a relic from a world before render props.

(<TabsState>
  {({ active, selected }) =>
    Object.keys(groups).map(groupName => (
      <KnobsTab
        key={groupName}
        active={active || selected === DEFAULT_GROUP_ID}
        name={groupName}
        knobs={knobsArray.filter(/* ... */)}
        /* ... */
      />
    ))
  }
</TabsState>)
function KnobsTab({ active, name, knobs /* ... */ }) {
  return (
    <Tab id={name} title={name}>
      <TabWrapper active={active}>
        ...
      </TabWrapper>
    </Tab>
  )
}

Copy link
Member

Choose a reason for hiding this comment

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

Would be happy to chat about how to improve this, Are you on the Storybook discord by any chance?

@igor-dv
Copy link
Member

igor-dv commented Dec 29, 2018

@tmeasday, @shilman, why does chromatic think there are changes?

@igor-dv igor-dv changed the base branch from master to next December 29, 2018 14:28
@igor-dv igor-dv changed the base branch from next to master December 29, 2018 14:28
@igor-dv
Copy link
Member

igor-dv commented Dec 29, 2018

Also it should be rebased on next

@ndelangen ndelangen changed the base branch from master to next January 4, 2019 18:04
@ndelangen ndelangen self-assigned this Jan 4, 2019
@ndelangen
Copy link
Member

I'll rebase this

@ndelangen ndelangen added this to the next milestone Jan 4, 2019
@codecov
Copy link

codecov bot commented Jan 4, 2019

Codecov Report

Merging #5106 into next will increase coverage by 0.29%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #5106      +/-   ##
==========================================
+ Coverage   35.02%   35.32%   +0.29%     
==========================================
  Files         596      596              
  Lines        7371     7375       +4     
  Branches     1005     1012       +7     
==========================================
+ Hits         2582     2605      +23     
+ Misses       4277     4260      -17     
+ Partials      512      510       -2
Impacted Files Coverage Δ
addons/knobs/src/components/Panel.js 81.57% <100%> (+17.69%) ⬆️
addons/knobs/src/components/PropForm.js 84.61% <0%> (+53.84%) ⬆️

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 a442dba...b31a405. Read the comment docs.

@ndelangen ndelangen force-pushed the fix-4829 branch 3 times, most recently from 268c9cf to b31a405 Compare January 4, 2019 18:50
@ndelangen
Copy link
Member

OK it's rebased, and I took the liberty of applying the changes for @igor-dv review comments.

@Kovensky we're migrating to typescript, and we're discussing how the future version of addon-knobs is going to look like / function on discord. I'd love your input on it, and help if you can. 🙇

@ndelangen ndelangen merged commit 5108da8 into storybookjs:next Jan 4, 2019
@Jessidhia Jessidhia deleted the fix-4829 branch January 7, 2019 09:02
@shilman shilman added patch:yes Bugfix & documentation PR that need to be picked to main branch patch:done Patch/release PRs already cherry-picked to main/release branch labels Jan 9, 2019
shilman pushed a commit that referenced this pull request Jan 9, 2019
Fix #4829: render non-grouped knobs in the ALL tab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: knobs bug 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants