-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Addons: Disable option for addon tab #6923
Addons: Disable option for addon tab #6923
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://monorepo-git-fork-jsomsanith-jsomsanith-featdisabletab.storybook.now.sh |
This seems reasonable to me |
Thanks @tmeasday, I'll work on the doc/test/kitchen sink |
lib/ui/src/containers/panel.js
Outdated
const mapper = ({ state, api }) => ({ | ||
panels: api.getPanels(), | ||
panels: filterPanel(api.getPanels(), api.getParameters(state.storyId)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should getFilteredPanels
be an api?
should getPanels
just return the filtered list
We should make sure this is memoized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a getPanelForStory(storyParameters)
in the addon api.
Since the api.getParameters()
is in the story api, it feels wrong to use it in the addon api. My first thought is that those are separate modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The modules addons, layout, stories are all interconnected.
It's OK to call methods on the final / complete api in methods of the SubAPI.
It's probably not a best practice or anything, but duplicating code and making work arounds so "modules" appear self-contained when in fact they aren't really is maybe even worse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and I haven't noticed it but the addon api already use other apis.
Changed it :)
@jsomsanith there seems to be a runtime error in the build versions, could you check it out? Please ask me for help, if you need it, happy to pair on it if you'd like. |
Awesome, does anyone else want to review before I merge? |
It’s been 15 days, when this PR will be merged ? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SOLID 👊
Issue: #6849
What I did
The issue above ask for a feature to disable a11y panel for a single story. But the idea of this PR is to provide a solution at a common level instead of at addon level.
The principle :
How to test
Addon panel
example has been addedIf your answer is yes to any of these, please make sure to include it in your PR.