fix: nested layout min width limit on Lg screen #5614
Conversation
JIRA: LX-668 risk: low
ivan-nejezchleb
force-pushed
the
ine-lx-668-2
branch
from
0d20bca
to
5af3649
Compare
| @@ -260,13 +266,15 @@ import { IWidgetAlert } from '@gooddata/sdk-model'; | |||
| import { IWidgetDefinition } from '@gooddata/sdk-model'; | |||
| import { IWorkspacePermissions } from '@gooddata/sdk-model'; | |||
| import { IWorkspaceUser } from '@gooddata/sdk-model'; | |||
| import { LayoutState as LayoutState_2 } from './layoutState.js'; | |||
There was a problem hiding this comment.
I'm not sure state and all the actions from line 5970 should be exposed in contract.
| layoutItemSize?: IDashboardLayoutSizeByScreenSize, | ||
| ): IDashboardLayoutSize => { | ||
| return { | ||
| ...(layoutItemSize ? layoutItemSize[screen] : {}), |
There was a problem hiding this comment.
there could still be a null pointer exc when layoutItemSize does not have size for provided screen. This should handle it:
...(layoutItemSize ? layoutItemSize[screen] ?? {} : {}),
| layoutItemSize?: IDashboardLayoutSizeByScreenSize, | ||
| ) => { | ||
| // Determine if element has size set in metadata object for the current screen size | ||
| const providedSizeForScreen = layoutItemSize ? layoutItemSize[screen] : undefined; |
There was a problem hiding this comment.
const providedSizeForScreen = layoutItemSize.?[screen]; is shorter (this is probably older code)
| @@ -181,9 +181,12 @@ export { selectLegacyDashboards } from "./legacyDashboards/legacyDashboardsSelec | |||
|
|
|||
| export type { UndoEnhancedState, UndoEntry } from "./_infra/undoEnhancer.js"; | |||
| export type { LayoutState, LayoutStash } from "./layout/layoutState.js"; | |||
|
|
|||
| export { layoutActions } from "./layout/index.js"; | |||
There was a problem hiding this comment.
Should this be exposed? I don't see other store actions being exposed directly.
There was a problem hiding this comment.
replaced by command+event combo
| <DashboardScreenSizeContext.Provider value={screenSize}> | ||
| {children} | ||
| {!appStateScreenSize ? null : children} |
There was a problem hiding this comment.
Please flip the condition to remove unnecessary negation.
| @@ -25,6 +37,20 @@ const getScreenSize = (ref: React.RefObject<HTMLDivElement>): ScreenSize => { | |||
| return "xs"; | |||
| }; | |||
|
|
|||
| function setNewSize( | |||
| screenSize: string | undefined, | |||
There was a problem hiding this comment.
maybe rename this to previousScreenSize?
| setScreenSize: React.Dispatch<React.SetStateAction<ScreenSize | undefined>>, | ||
| dispatch: React.Dispatch<AnyAction>, | ||
| ) { | ||
| const newSize = getScreenSize(); |
There was a problem hiding this comment.
possibly change this to const currentScreenSize = detectCurrentScreenSize(); or getCurrentScreenSize
ivan-nejezchleb
force-pushed
the
ine-lx-668-2
branch
from
9562648
to
b4bee1d
Compare
Nested layout width limit was not working on Lg screen. Screen size was needed also in sagas so context was not enough. Screen provider syncs screen size to app state. Screen size provider rewritten to measure whole window not its container. It is not optimal for embedding use case, but needed for KD compatibility. JIRA: LX-668 risk: low
ivan-nejezchleb
force-pushed
the
ine-lx-668-2
branch
from
b4bee1d
to
3fb03e4
Compare
Important
Please, don't forget to run
rush changefor the commits that introduce new features 🙏Refer to documentation to see how to run checks and tests in the pull request. This is the list of the most used commands: