-
-
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
UI: Add storyStatus to sidebar UI #23342
Conversation
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.
Works great!
Do we have a design for what should happen in the search? Do statuses show up @MichaelArestad?
Either way I think it'd be good to add some tests for search results with + without statuses.
data-selected={isSelected} | ||
data-ref-id={refId} | ||
data-item-id={item.id} | ||
data-parent-id={item.parent} | ||
data-nodetype={item.type === 'docs' ? 'document' : 'story'} | ||
data-highlightable={isDisplayed} | ||
className="sidebar-item" |
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.
Is this a breaking change? I assume you do this so the attributes wrap the icon/tooltip. You could alternatively just put the icon/tooltip inside the DocumentNode
/StoryNode
?
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 didn't consider it a breaking change.. I had to make it because the leaf-node is an anchor tag with a href.
I can't put another button or anchor inside, that's invalid.
So I moved these attributes and background styling up to the wrapper, made that display:flex
and added the icon (it's clickable!) besides the link.
{(item.renderLabel as (i: typeof item) => React.ReactNode)?.(item) || item.name} | ||
</BranchNode> | ||
); | ||
if (item.type === 'component' || item.type === 'group') { |
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.
Isn't this check redundant? TS says so. I would prefer to throw if the runtime type doesn't match, don't you think?
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.
Yes it's redundant, I thought it made the code more clear.
Are you saying I should remove the if-statement, or add a throw
at the end, if there were no matches?
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.
If you think it's clearer, I guess that's good. But yeah, I think throwing at the end rather than null
is probably better if TS thinks its impossible. WDYT?
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.
It should be impossible, but I see no reason to risk throwing an error. Because throwing here would mean react completely unmounts the whole manager app.
AFAIK the search shouldn't show status |
…ybook into norbert/story-status-ui
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.
LGTM if @MichaelArestad can confirm:
AFAIK the search shouldn't show status
@MichaelArestad If you think I should add indicators for the search UI, or you'd like to iterate on the exact design details, I'd love to do that in a follow-up PR. |
@ndelangen Thanks. I need time to specify what we should do with search results. Right now, I think you made the right choice to move on. |
Is there any documentation on how to use this feature? |
No, It's still marked as experimental, for now. We'll revisit the documentation in 9.0. |
What I did
How to test
manager.ts
inui/.storybook
tomanager.tsx
Checklist
MIGRATION.MD
Maintainers
make sure to add the
ci:merged
orci:daily
GH label to it.["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]