-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: New tab ui changes #33228
feat: New tab ui changes #33228
Changes from 23 commits
bf1f488
d84c5d0
79f4185
12534de
bee5670
272561a
b2fa38b
ef30093
ab1e270
ae5654e
97feeb5
1012f63
e086e0f
6d63f0d
2da706b
cd2212b
2e3629d
5ac67bc
dd72f03
8d0a0ee
c0f210a
974b375
9540f0c
af6df38
36faf74
5c0b56d
bff3ef1
54dda9b
6debce9
6dff7ef
6eb7205
ea658d0
48223f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import { ObjectsRegistry } from "../../Objects/Registry"; | ||
import { PagePaneSegment } from "../EditorNavigation"; | ||
import AddView from "./AddView"; | ||
import FileTabs from "./FileTabs"; | ||
import ListView from "./ListView"; | ||
|
||
export class LeftPane { | ||
|
@@ -112,12 +114,20 @@ export class LeftPane { | |
this.addView.assertInAddView(); | ||
} | ||
|
||
public closeAddView() { | ||
this.addView.closeAddView(); | ||
public closeAddView(segment: PagePaneSegment) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This argument here is an anti-pattern. If we are seeing that the closing of the add view is different based on segments, it should be added as behaviours or exposed via sub classes |
||
if (!this.segments) { | ||
throw Error("No Segments configured"); | ||
} | ||
cy.log(segment, "===Albin"); | ||
if (segment === PagePaneSegment.UI) { | ||
this.addView.closeAddView(); | ||
} else { | ||
FileTabs.closeTab("new"); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Codebase Verification Several files in the codebase still use the old function signature.
Analysis chainIntroduced a new method Scripts ExecutedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that all segments are correctly handled in the `closeAddView` method.
# Test: Check the method implementation for handling different segments.
cat app/client/cypress/support/Pages/IDE/LeftPane.ts | grep -A 10 "closeAddView"
Length of output: 482 Script: #!/bin/bash
# Description: Check the definition of the enum `PagePaneSegment` to verify all possible segments.
# Test: Retrieve the definition of `PagePaneSegment`.
ast-grep --lang typescript --pattern 'enum PagePaneSegment {$$$}'
Length of output: 438 |
||
} | ||
|
||
public getCreateOptions() { | ||
return this.addView.getCreateOptions(); | ||
public getCreateOptions(name: string) { | ||
return this.addView.getCreateOptions(name); | ||
} | ||
|
||
public assertInListView() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,28 +32,28 @@ export const useJSAdd = () => { | |
(opt) => opt.focusEntityType === FocusEntity.JS_MODULE_INSTANCE, | ||
); | ||
|
||
return useCallback(() => { | ||
if (jsModuleCreationOptions.length === 0) { | ||
if (segmentMode === EditorEntityTabState.Add) { | ||
const url = getJSUrl(currentEntityInfo, false); | ||
history.push(url); | ||
return useCallback( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing a boolean argument here adds more complication to an already complicated hook. I strongly believe we need to refactor this to be more explicit |
||
(add: boolean) => { | ||
if (jsModuleCreationOptions.length === 0) { | ||
if (segmentMode === EditorEntityTabState.Add) { | ||
const url = getJSUrl(currentEntityInfo, false); | ||
history.push(url); | ||
} else { | ||
dispatch(createNewJSCollection(pageId, "ENTITY_EXPLORER")); | ||
} | ||
} else { | ||
dispatch(createNewJSCollection(pageId, "ENTITY_EXPLORER")); | ||
if (currentEntityInfo.entity === FocusEntity.JS_OBJECT_ADD && add) { | ||
return; | ||
} | ||
const url = getJSUrl( | ||
currentEntityInfo, | ||
!(segmentMode === EditorEntityTabState.Add), | ||
); | ||
history.push(url); | ||
} | ||
} else { | ||
const url = getJSUrl( | ||
currentEntityInfo, | ||
!(segmentMode === EditorEntityTabState.Add), | ||
); | ||
history.push(url); | ||
} | ||
}, [ | ||
dispatch, | ||
pageId, | ||
segmentMode, | ||
currentEntityInfo, | ||
jsModuleCreationOptions, | ||
]); | ||
}, | ||
[dispatch, pageId, segmentMode, currentEntityInfo, jsModuleCreationOptions], | ||
); | ||
}; | ||
|
||
export const useIsJSAddLoading = () => { | ||
|
@@ -111,17 +111,11 @@ export const useJSSegmentRoutes = (path: string): UseRoutes => { | |
]; | ||
} | ||
return [ | ||
{ | ||
exact: true, | ||
key: "AddJS", | ||
component: AddJS, | ||
path: [`${path}${ADD_PATH}`, `${path}/:collectionId${ADD_PATH}`], | ||
}, | ||
{ | ||
exact: false, | ||
key: "ListJS", | ||
component: ListJS, | ||
path: [path], | ||
path: [path, `${path}${ADD_PATH}`, `${path}/:collectionId${ADD_PATH}`], | ||
}, | ||
]; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,7 @@ describe("JS Segment", () => { | |
expect(result.current).toBeDefined(); | ||
|
||
act(() => { | ||
result.current(); | ||
result.current(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding assertions to verify the state before and after the dispatch to ensure the state is updated as expected. |
||
}); | ||
|
||
expect(mockDispatchFn).toBeCalledWith({ | ||
|
@@ -66,7 +66,7 @@ describe("JS Segment", () => { | |
|
||
// Call the function now from a different page | ||
act(() => { | ||
result.current(); | ||
result.current(true); | ||
}); | ||
|
||
// Now the creation action should have the new page id | ||
|
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.
Tip
Codebase Verification
The search results indicate that the
data-testid='t--ide-add-pane'
is used in multiple TypeScript files within the project. This suggests that thedata-testid
is not unique, which could potentially lead to issues in the test scripts where this identifier is expected to be unique for specific elements.Files where
data-testid='t--ide-add-pane'
is found:app/client/src/pages/Editor/IDE/EditorPane/UI/Add.tsx
app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx
app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx
Given this information, it is clear that the
data-testid
is not uniquely implemented across the project, which contradicts the requirement for uniqueness to avoid test failures.Analysis chain
Added a new locator for the add pane. Ensure that the
data-testid
is unique and correctly implemented in the corresponding HTML element to avoid test failures.Scripts Executed
The following scripts were executed for the analysis:
Script:
Length of output: 48
Script:
Length of output: 331