Skip to content

chore: Remove empty canvas prompts and improve widget editor header #33993

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.

Sign up for GitHub

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

Merged
rahulbarwal merged 5 commits into from
Jun 6, 2024
Merged

chore: Remove empty canvas prompts and improve widget editor header #33993

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4302d10
removes empty canvas prompts in favour of drag and drop building blocks.
rahulbarwal Jun 5, 2024
f72d02a
fixes spec by accomodating removal of empty canvas CTA.
rahulbarwal Jun 5, 2024
846e3c0
Update test to add new page from template in Fork_Template_To_App_spe…
rahulbarwal Jun 5, 2024
449f19a
Minor fix: forgot to remove`.first()`
rahulbarwal Jun 6, 2024
5863f53
Minor fix: use correct `agHelper` using import.
rahulbarwal Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,10 @@ describe(
});

it("1.Bug #17002 Forking a template into an existing app which is connected to git makes the application go into a bad state ", function () {
cy.get(template.startFromTemplateCard).click();
_.assertHelper.AssertNetworkStatus("fetchTemplate");

cy.get(template.templateDialogBox).should("be.visible");
cy.get(template.templateCard).first().click();
cy.get(template.templateViewForkButton).first().click();
PageList.AddNewPage("Add page from template");
agHelper.AssertElementExist(template.templateDialogBox);
_.agHelper.GetNClick(template.templateCard);
_.agHelper.GetNClick(template.templateViewForkButton);
Comment on lines +35 to +38

@coderabbitai coderabbitai Bot Jun 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updates to use custom functions and helper methods enhance the interaction with page elements. Consider converting these function expressions to arrow functions for better readability and consistency.

- function () {
+ () => {

Committable suggestion was skipped due to low confidence.

@rahulbarwal rahulbarwal Jun 6, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai I don't understand this. Can you explain more?

@coderabbitai coderabbitai Bot Jun 6, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rahulbarwal, I suggested converting function expressions to arrow functions primarily for syntactic cleanliness and consistency. Arrow functions offer a more concise syntax and do not have their own this context, which can simplify code, especially when dealing with nested functions.

However, in the context of Cypress tests, it's important to note that using arrow functions might not always be appropriate. Cypress uses the function() syntax for test callbacks to bind its own context (like this.timeout or this.retries) to these functions. Using arrow functions, which don't bind their own this, could potentially lead to issues if you rely on Cypress's custom context.

Therefore, while arrow functions are generally a good practice for cleaner and more concise code, they should be used cautiously within Cypress tests where traditional function expressions might be necessary to access Cypress-specific features tied to this. I hope this clarifies the suggestion! If you have any more questions or need further assistance, feel free to ask.

Learnings used 
User: dipyamanbiswas07
PR: appsmithorg/appsmith#29282
File: app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js:11-17
Timestamp: 2024-01-09T08:39:59.723Z
Learning: The user has clarified that intercepts are called in end-to-end tests, and therefore resetting the feature flag in an `afterEach` hook is not required in their test setup.

@rahulbarwal rahulbarwal Jun 6, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai I still don't see where is the opportunity to replace the function expression to arrow function.

@coderabbitai coderabbitai Bot Jun 6, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rahulbarwal, thank you for your patience. Upon reviewing the context of the Cypress tests in the file ForkTemplateToGitConnectedApp.js, I realize that the suggestion to convert function expressions to arrow functions was not appropriate.

In Cypress, it's recommended to use traditional function expressions for test definitions (it blocks). This is because Cypress extends the function's this context with its own set of properties and methods (like this.timeout or this.retries). Arrow functions, which do not have their own this context, would not be able to access these Cypress-specific features, potentially leading to issues in your tests.

Therefore, in the context of your Cypress tests, it's best to continue using function expressions as you currently are. The initial suggestion was more of a general coding practice and didn't account for the specific requirements and recommendations of writing Cypress tests. I appreciate your diligence in seeking clarification on this! If there's anything else you need, feel free to ask.

Learnings used 
User: dipyamanbiswas07
PR: appsmithorg/appsmith#29282
File: app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js:11-17
Timestamp: 2024-01-09T08:39:59.723Z
Learning: The user has clarified that intercepts are called in end-to-end tests, and therefore resetting the feature flag in an `afterEach` hook is not required in their test setup.

cy.waitUntil(() => cy.xpath("//span[text()='Setting up the template']"), {
errorMsg: "Setting Templates did not finish even after 75 seconds",
timeout: 75000,
Expand Down
2 changes: 1 addition & 1 deletion app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_To_App_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe(

it("2. Add selected pages from template to an app", () => {
homePage.CreateNewApplication();
agHelper.GetNClick(template.startFromTemplateCard);
PageList.AddNewPage("Add page from template");
agHelper.AssertElementVisibility(template.templateDialogBox);
agHelper.GetNClick("//h1[text()='Applicant Tracker-test']");
agHelper.FailIfErrorToast(
Expand Down
6 changes: 0 additions & 6 deletions app/client/src/ce/constants/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1771,9 +1771,6 @@ export const WIDGET_USED = () => "Widgets";
export const SIMILAR_TEMPLATES = () => "Similar templates";
export const VIEW_ALL_TEMPLATES = () => "View all templates";
export const FILTERS = () => "Filters";
export const TEMPLATE_CARD_TITLE = () => "Start from a template";
export const TEMPLATE_CARD_DESCRIPTION = () =>
"Create app from template by selecting pages";
export const FILTER_SELECTALL = () => "Select all";
export const FILTER_SELECT_PAGE = () => "Add selected page";
export const FILTER_SELECT_PAGES = () => "Add selected pages";
Expand Down Expand Up @@ -1826,9 +1823,6 @@ export const SEARCH_USERS = (

export const CREATE_PAGE = () => "New blank page";
export const CANVAS_NEW_PAGE_CARD = () => "Create new page";
export const GENERATE_PAGE = () => "Generate page from data table";
export const GENERATE_PAGE_DESCRIPTION = () =>
"Start app with a simple CRUD UI and customize it";
export const ADD_PAGE_FROM_TEMPLATE = () => "Add page from template";
export const INVALID_URL = () =>
"Please enter a valid URL, for example, https://example.com";
Expand Down
20 changes: 2 additions & 18 deletions app/client/src/pages/Editor/WidgetsEditor/WidgetEditorHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
import React from "react";
import AnonymousDataPopup from "pages/Editor/FirstTimeUserOnboarding/AnonymousDataPopup";
import EmptyCanvasPrompts from "./components/EmptyCanvasPrompts";
import { useSelector } from "react-redux";
import { combinedPreviewModeSelector } from "selectors/editorSelectors";
import { getIsAppSettingsPaneWithNavigationTabOpen } from "selectors/appSettingsPaneSelectors";
import { useCurrentAppState } from "pages/Editor/IDE/hooks";
import { EditorState } from "@appsmith/entities/IDE/constants";
import useMissingModuleNotification from "@appsmith/pages/Editor/IDE/MainPane/useMissingModuleNotification";
import AnonymousDataPopup from "pages/Editor/FirstTimeUserOnboarding/AnonymousDataPopup";
import React from "react";

/**
* WidgetEditorHeader
Expand All @@ -17,19 +11,9 @@ import useMissingModuleNotification from "@appsmith/pages/Editor/IDE/MainPane/us
* - missing module notification
*/
export const WidgetEditorHeader = () => {
const isNavigationSelectedInSettings = useSelector(
getIsAppSettingsPaneWithNavigationTabOpen,
);
const appState = useCurrentAppState();
const isAppSettingsPaneWithNavigationTabOpen =
appState === EditorState.SETTINGS && isNavigationSelectedInSettings;
const isPreviewMode = useSelector(combinedPreviewModeSelector);
const missingModuleNotification = useMissingModuleNotification();
return (
<>
{!isAppSettingsPaneWithNavigationTabOpen && (
<EmptyCanvasPrompts isPreview={isPreviewMode} />
)}
{missingModuleNotification}
<AnonymousDataPopup />
</>
Expand Down
170 changes: 0 additions & 170 deletions app/client/src/pages/Editor/WidgetsEditor/components/EmptyCanvasPrompts.tsx
View file
Open in desktop

This file was deleted.