From 5041a105338f0d4f7a8da99ea69f5a2ff140d978 Mon Sep 17 00:00:00 2001
From: Ramesh Mane <101566080+rameshmane7218@users.noreply.github.com>
Date: Fri, 5 Jul 2024 16:43:13 +0530
Subject: [PATCH] Fix: Persist the left sidebar width and use pixels instead of
 percentage to configure the sidebar width (#8931)

* fix(nc-gui): use px to configure left sidebar width instead of percentage

* fix(nc-gui): ai review changes

* fix(nc-gui): use 256px sidebar default width in pw testing

* fix: use constant sidebar width

* fix(test): airtable import test fail issue

* chore(test): remove console

* chore(nc-gui): update comment
---
 packages/nc-gui/components/dashboard/View.vue | 63 ++++++++++---------
 .../nc-gui/composables/useGlobal/actions.ts   |  5 ++
 .../nc-gui/composables/useGlobal/state.ts     |  2 +
 .../nc-gui/composables/useGlobal/types.ts     |  2 +
 packages/nc-gui/lib/constants.ts              |  2 +
 packages/nc-gui/store/sidebar.ts              | 29 ++++++---
 .../pages/Dashboard/Grid/Column/index.ts      |  5 +-
 tests/playwright/quickTests/commonTest.ts     |  8 ++-
 8 files changed, 75 insertions(+), 41 deletions(-)

diff --git a/packages/nc-gui/components/dashboard/View.vue b/packages/nc-gui/components/dashboard/View.vue
index 429db70c3a..49478ad30b 100644
--- a/packages/nc-gui/components/dashboard/View.vue
+++ b/packages/nc-gui/components/dashboard/View.vue
@@ -5,6 +5,8 @@ import 'splitpanes/dist/splitpanes.css'
 const router = useRouter()
 const route = router.currentRoute
 
+const { setLeftSidebarSize } = useGlobal()
+
 const { isMobileMode } = storeToRefs(useConfigStore())
 
 const {
@@ -30,22 +32,32 @@ const currentSidebarSize = computed({
 
 const { handleSidebarOpenOnMobileForNonViews } = useConfigStore()
 
-const contentSize = computed(() => 100 - sideBarSize.value.current)
-
 const mobileNormalizedContentSize = computed(() => {
   if (isMobileMode.value) {
     return isLeftSidebarOpen.value ? 0 : 100
   }
 
-  return contentSize.value
+  return 100 - leftSidebarWidthPercent.value
 })
 
-const sidebarWidth = computed(() =>
-  isMobileMode.value ? viewportWidth.value : (sideBarSize.value.old * viewportWidth.value) / 100,
-)
-
 watch(currentSidebarSize, () => {
-  leftSidebarWidthPercent.value = currentSidebarSize.value
+  leftSidebarWidthPercent.value = (currentSidebarSize.value / viewportWidth.value) * 100
+  setLeftSidebarSize(currentSidebarSize.value)
+})
+
+const sidebarWidth = computed(() => (isMobileMode.value ? viewportWidth.value : sideBarSize.value.old))
+
+const normalizedWidth = computed(() => {
+  const maxSize = remToPx(viewportWidth.value <= 1560 ? 20 : 35)
+  const minSize = remToPx(16)
+
+  if (sidebarWidth.value > maxSize) {
+    return maxSize
+  } else if (sidebarWidth.value < minSize) {
+    return minSize
+  } else {
+    return sidebarWidth.value
+  }
 })
 
 watch(isLeftSidebarOpen, () => {
@@ -87,10 +99,15 @@ function handleMouseMove(e: MouseEvent) {
   }
 }
 
-function onWindowResize() {
+function onWindowResize(e?: any): void {
   viewportWidth.value = window.innerWidth
 
-  onResize(currentSidebarSize.value)
+  leftSidebarWidthPercent.value = (currentSidebarSize.value / viewportWidth.value) * 100
+
+  // if sidebar width is greater than normalized width and this function is called from window resize event (not from template) update left sidebar width
+  if (e && normalizedWidth.value < sidebarWidth.value) {
+    onResize(leftSidebarWidthPercent.value)
+  }
 }
 
 onMounted(() => {
@@ -138,10 +155,9 @@ function onResize(widthPercent: any) {
   const fontSize = parseFloat(getComputedStyle(document.documentElement).fontSize)
 
   // If the viewport width is less than 1560px, the max sidebar width should be 20rem
-
   if (viewportWidth.value <= 1560) {
     if (width > remToPx(20)) {
-      sideBarSize.value.old = ((20 * fontSize) / viewportWidth.value) * 100
+      sideBarSize.value.old = 20 * fontSize
       if (isLeftSidebarOpen.value) sideBarSize.value.current = sideBarSize.value.old
       return
     }
@@ -150,31 +166,19 @@ function onResize(widthPercent: any) {
   const widthRem = width / fontSize
 
   if (widthRem < 16) {
-    sideBarSize.value.old = ((16 * fontSize) / viewportWidth.value) * 100
+    sideBarSize.value.old = 16 * fontSize
     if (isLeftSidebarOpen.value) sideBarSize.value.current = sideBarSize.value.old
     return
   } else if (widthRem > 35) {
-    sideBarSize.value.old = ((35 * fontSize) / viewportWidth.value) * 100
+    sideBarSize.value.old = 35 * fontSize
     if (isLeftSidebarOpen.value) sideBarSize.value.current = sideBarSize.value.old
 
     return
   }
 
-  sideBarSize.value.old = widthPercent
+  sideBarSize.value.old = width
   sideBarSize.value.current = sideBarSize.value.old
 }
-
-const normalizedWidth = computed(() => {
-  const maxSize = remToPx(35)
-  const minSize = remToPx(16)
-  if (sidebarWidth.value > maxSize) {
-    return maxSize
-  } else if (sidebarWidth.value < minSize) {
-    return minSize
-  } else {
-    return sidebarWidth.value
-  }
-})
 </script>
 
 <template>
@@ -192,7 +196,8 @@ const normalizedWidth = computed(() => {
       max-size="60%"
       class="nc-sidebar-splitpane !sm:max-w-140 relative !overflow-visible flex"
       :style="{
-        width: `${mobileNormalizedSidebarSize}%`,
+        'width': `${mobileNormalizedSidebarSize}%`,
+        'min-width': `${mobileNormalizedSidebarSize}%`,
       }"
     >
       <div
@@ -215,7 +220,7 @@ const normalizedWidth = computed(() => {
       :size="mobileNormalizedContentSize"
       class="flex-grow"
       :style="{
-        'min-width': `${100 - mobileNormalizedSidebarSize}%`,
+        'min-width': `${mobileNormalizedContentSize}%`,
       }"
     >
       <slot name="content" />
diff --git a/packages/nc-gui/composables/useGlobal/actions.ts b/packages/nc-gui/composables/useGlobal/actions.ts
index 8056860eb1..644cf45644 100644
--- a/packages/nc-gui/composables/useGlobal/actions.ts
+++ b/packages/nc-gui/composables/useGlobal/actions.ts
@@ -160,6 +160,10 @@ export function useGlobalActions(state: State): Actions {
     state.gridViewPageSize.value = pageSize
   }
 
+  const setLeftSidebarSize = (size: number) => {
+    state.leftSidebarSize.value = size
+  }
+
   return {
     signIn,
     signOut,
@@ -171,5 +175,6 @@ export function useGlobalActions(state: State): Actions {
     ncNavigateTo,
     getMainUrl,
     setGridViewPageSize,
+    setLeftSidebarSize,
   }
 }
diff --git a/packages/nc-gui/composables/useGlobal/state.ts b/packages/nc-gui/composables/useGlobal/state.ts
index 0606abec8c..eb8c6938bf 100644
--- a/packages/nc-gui/composables/useGlobal/state.ts
+++ b/packages/nc-gui/composables/useGlobal/state.ts
@@ -1,6 +1,7 @@
 import { useStorage } from '@vueuse/core'
 import type { JwtPayload } from 'jwt-decode'
 import type { AppInfo, State, StoredState } from './types'
+import { INITIAL_LEFT_SIDEBAR_WIDTH } from '~/lib/constants'
 
 export function useGlobalState(storageKey = 'nocodb-gui-v2'): State {
   /** get the preferred languages of a user, according to browser settings */
@@ -57,6 +58,7 @@ export function useGlobalState(storageKey = 'nocodb-gui-v2'): State {
     isMobileMode: null,
     lastOpenedWorkspaceId: null,
     gridViewPageSize: 25,
+    leftSidebarSize: INITIAL_LEFT_SIDEBAR_WIDTH,
   }
 
   /** saves a reactive state, any change to these values will write/delete to localStorage */
diff --git a/packages/nc-gui/composables/useGlobal/types.ts b/packages/nc-gui/composables/useGlobal/types.ts
index 6a107647dc..17ff0fec92 100644
--- a/packages/nc-gui/composables/useGlobal/types.ts
+++ b/packages/nc-gui/composables/useGlobal/types.ts
@@ -53,6 +53,7 @@ export interface StoredState {
   isMobileMode: boolean | null
   lastOpenedWorkspaceId: string | null
   gridViewPageSize: number
+  leftSidebarSize: number
 }
 
 export type State = ToRefs<Omit<StoredState, 'token'>> & {
@@ -89,6 +90,7 @@ export interface Actions {
   getBaseUrl: (workspaceId: string) => string | undefined
   getMainUrl: (workspaceId: string) => string | undefined
   setGridViewPageSize: (pageSize: number) => void
+  setLeftSidebarSize: (size: number) => void
 }
 
 export type ReadonlyState = Readonly<Pick<State, 'token' | 'user'>> & Omit<State, 'token' | 'user'>
diff --git a/packages/nc-gui/lib/constants.ts b/packages/nc-gui/lib/constants.ts
index f8257bebfa..a73882c6cd 100644
--- a/packages/nc-gui/lib/constants.ts
+++ b/packages/nc-gui/lib/constants.ts
@@ -18,3 +18,5 @@ export const GROUP_BY_VARS = {
     __nc_false__: 'Unchecked',
   } as Record<string, string>,
 }
+
+export const INITIAL_LEFT_SIDEBAR_WIDTH = 288
diff --git a/packages/nc-gui/store/sidebar.ts b/packages/nc-gui/store/sidebar.ts
index efab8236cf..b37a9386ed 100644
--- a/packages/nc-gui/store/sidebar.ts
+++ b/packages/nc-gui/store/sidebar.ts
@@ -1,12 +1,12 @@
 import { acceptHMRUpdate, defineStore } from 'pinia'
-import { MAX_WIDTH_FOR_MOBILE_MODE } from '~/lib/constants'
+import { MAX_WIDTH_FOR_MOBILE_MODE, INITIAL_LEFT_SIDEBAR_WIDTH } from '~/lib/constants'
 
 export const useSidebarStore = defineStore('sidebarStore', () => {
   const { width } = useWindowSize()
   const isViewPortMobile = () => {
     return width.value < MAX_WIDTH_FOR_MOBILE_MODE
   }
-  const { isMobileMode } = useGlobal()
+  const { isMobileMode, leftSidebarSize: _leftSidebarSize, setLeftSidebarSize } = useGlobal()
 
   const tablesStore = useTablesStore()
   const _isLeftSidebarOpen = ref(!isViewPortMobile())
@@ -21,13 +21,13 @@ export const useSidebarStore = defineStore('sidebarStore', () => {
 
   const isRightSidebarOpen = ref(true)
 
-  const leftSidebarWidthPercent = ref(isViewPortMobile() ? 0 : 20)
-
   const leftSideBarSize = ref({
-    old: 20,
-    current: leftSidebarWidthPercent.value,
+    old: _leftSidebarSize.value ?? INITIAL_LEFT_SIDEBAR_WIDTH,
+    current: isViewPortMobile() ? 0 : _leftSidebarSize.value ?? INITIAL_LEFT_SIDEBAR_WIDTH,
   })
 
+  const leftSidebarWidthPercent = ref((leftSideBarSize.value.current / width.value) * 100)
+
   const leftSidebarState = ref<
     'openStart' | 'openEnd' | 'hiddenStart' | 'hiddenEnd' | 'peekOpenStart' | 'peekOpenEnd' | 'peekCloseOpen' | 'peekCloseEnd'
   >(isLeftSidebarOpen.value ? 'openEnd' : 'hiddenEnd')
@@ -37,10 +37,16 @@ export const useSidebarStore = defineStore('sidebarStore', () => {
       return isLeftSidebarOpen.value ? 100 : 0
     }
 
-    return leftSideBarSize.value.current
+    return leftSidebarWidthPercent.value
   })
 
-  const leftSidebarWidth = computed(() => (width.value * mobileNormalizedSidebarSize.value) / 100)
+  const leftSidebarWidth = computed(() => {
+    if (isMobileMode.value) {
+      return isLeftSidebarOpen.value ? width.value : 0
+    }
+
+    return leftSideBarSize.value.current
+  })
 
   const nonHiddenMobileSidebarSize = computed(() => {
     if (isMobileMode.value) {
@@ -50,7 +56,12 @@ export const useSidebarStore = defineStore('sidebarStore', () => {
     return leftSideBarSize.value.current ?? leftSideBarSize.value.old
   })
 
-  const nonHiddenLeftSidebarWidth = computed(() => (width.value * nonHiddenMobileSidebarSize.value) / 100)
+  const nonHiddenLeftSidebarWidth = computed(() => {
+    if (isMobileMode.value) {
+      return width.value
+    }
+    return nonHiddenMobileSidebarSize.value
+  })
 
   const formRightSidebarState = ref({
     minWidth: 384,
diff --git a/tests/playwright/pages/Dashboard/Grid/Column/index.ts b/tests/playwright/pages/Dashboard/Grid/Column/index.ts
index ea7ef1f0c1..62a31c3530 100644
--- a/tests/playwright/pages/Dashboard/Grid/Column/index.ts
+++ b/tests/playwright/pages/Dashboard/Grid/Column/index.ts
@@ -461,10 +461,13 @@ export class ColumnPageObject extends BasePage {
     await this.rootPage.waitForTimeout(200);
   }
 
-  async verify({ title, isVisible = true }: { title: string; isVisible?: boolean }) {
+  async verify({ title, isVisible = true, scroll = false }: { title: string; isVisible?: boolean; scroll?: boolean }) {
     if (!isVisible) {
       return await expect(this.getColumnHeader(title)).not.toBeVisible();
     }
+    if (scroll) {
+      await this.getColumnHeader(title).scrollIntoViewIfNeeded();
+    }
     await expect(this.getColumnHeader(title)).toContainText(title);
   }
 
diff --git a/tests/playwright/quickTests/commonTest.ts b/tests/playwright/quickTests/commonTest.ts
index 6a98366c75..186d3cf66c 100644
--- a/tests/playwright/quickTests/commonTest.ts
+++ b/tests/playwright/quickTests/commonTest.ts
@@ -76,12 +76,16 @@ const quickVerify = async ({
     columnCount -= 3;
   }
   for (let i = 0; i < columnCount; i++) {
-    await dashboard.grid.column.verify({ title: cn[i] });
+    await dashboard.grid.column.verify({ title: cn[i], scroll: true });
   }
 
   // Verify cells
   // normal cells
-  for (const [key, value] of Object.entries(recordCells)) {
+  for (const [index, [key, value]] of Object.entries(recordCells).entries()) {
+    if (index === 0) {
+      await dashboard.grid.cell.get({ index: index, columnHeader: key }).click();
+    }
+
     await dashboard.grid.cell.verify({ index: cellIndex, columnHeader: key, value });
   }