Converged Side Pane & Control Bar#2976
Conversation
…s-side-pane-converged
Calling bundle size is increased❗.
|
CallWithChat bundle size is increased❗.
|
Chat bundle size is increased❗.
|
|
Failed to pass the composite UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Failed to pass the composite UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Failed to pass the composite UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
JamesBurnside
changed the title
JamesBurnside
changed the title
…s-side-pane-converged
| * @private | ||
| */ | ||
| export const SidePaneProvider = (props: SidePaneProviderProps): JSX.Element => { | ||
| const [headerRenderer, setHeaderRenderer] = React.useState<(() => JSX.Element) | undefined>(); |
There was a problem hiding this comment.
Is it possible to avoid adding global changing state into provider, it's a huge review push back to do so long time ago - it's another format of global state - our centralized state will not be single source of truth of global state anymore.
The conclusion of the discussion was we could use Provider for global static, but we shouldn't put any dynamic changing state there for either debuggability or code structure. It feels this is opening a new pattern that brings us back to that design
if we feel that is a need to add these states into adapter, I will 100% vote for that
|
|
||
| const [showDrawer, setShowDrawer] = useState(false); | ||
| const onMoreButtonClicked = useCallback(() => { | ||
| closePane(); |
There was a problem hiding this comment.
is this a intended new more button behaviour? do we want to keep the side pane open when going on hold for example? see people come and go, and be able to chat?
There was a problem hiding this comment.
Chatted with @dmceachernmsft , this behavior (because of a regression in main branch) does not happen today so I will not fix in this PR, but will do a subsequent PR to fix this.
…s-side-pane-converged
…s-side-pane-converged
| sidePaneId: string; | ||
| // Useful to ensure the side pane renders the content of the override even if the side pane is closed. | ||
| // This avoids remounting the content when the side pane is opened again. | ||
| hidden?: boolean; |
There was a problem hiding this comment.
disposeWhenClose?
hidden feels a bit more like UI behahvior like css "display: hidden" (and people will try to change it to true to show it)
There was a problem hiding this comment.
100% agree, updated to better names (isActive and persistRenderingWhenClosed):
| export const SidePaneProvider = (props: SidePaneProviderProps): JSX.Element => { | ||
| const [headerRenderer, setHeaderRenderer] = React.useState<(() => JSX.Element) | undefined>(); | ||
| const [contentRenderer, setContentRenderer] = React.useState<(() => JSX.Element) | undefined>(); | ||
| const [activeSidePaneId, setActiveSidePaneId] = React.useState<string>(); |
There was a problem hiding this comment.
Discussion from last time - we could still use provider, but just not to add state here
So this provider could take props(either states or normal props) from outside of the composite, then store it here
Or we just remove provider then use a top to down state from composite to each single component
There was a problem hiding this comment.
Updated for provider to take props for now
|
Failed to pass the composite UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
4 participants
What
Why
Remove much much duplicate usage of components and remove that call with chat composite had its own control bar and its own side pane.
This also fixed several bugs in the callcompsites new control bar:
How Tested
Lots across calling and callwithchat, stab and beta, mobile and landscape - but will also organize a bug bash on this as this impacts stable builds.