-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Move command bar out of experiments and into 6.0 #3138
Conversation
Apologies for the file diffs. It's similar enough that git tries to show what changed, but its so completely different that the diffs are almost meaningless |
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.
A couple minor things from scanning over it, but my main concern is that we're breaking an API (even if mildly, it's still breaking it) in a minor version update.
contextualMenuProps?: IContextualMenuProps; | ||
contextualMenuTarget?: HTMLElement; | ||
renderedFarItems?: IContextualMenuItem[]; | ||
export interface ICommandBarData { |
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.
Why not put this in the props file? As it is, you have a circular dependency between this file and the props file.
data={ commandBardata } | ||
onReduceData={ onReduceData } | ||
onGrowData={ onGrowData } | ||
// tslint:disable-next-line:jsx-no-lambda |
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.
Rather than disable TSLint, why not refactor this out to a function or a functional component?
It looks like there are actual visual regressions here; I don't think you intended to change paddings? |
Padding is the same, image is just zoomed since it's shorter. no search bar. |
This has been sitting in experiments far too long. Initial integration into outlook seemed quite good. I'd like to get this moving now that the dust is starting to settle with 5.0.