Skip to content
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

Jump to bottom

fix: Theming Selected new Font is reverted whenever Color is removed #36119

Merged
merged 6 commits into from
Sep 6, 2024
Merged

fix: Theming Selected new Font is reverted whenever Color is removed #36119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
efa3a5b
fix bug 15007 + update test
Sep 4, 2024
4d6b70e
update test
Sep 5, 2024
ad2c6a6
fix prettier error
Sep 5, 2024
3f7e7b6
update limited-tests.txt
Sep 5, 2024
8337791
update selectors
Sep 5, 2024
dec2147
fix prettier error
Sep 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
103 changes: 30 additions & 73 deletions app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const commonlocators = require("../../../../locators/commonlocators.json");
const themelocator = require("../../../../locators/ThemeLocators.json");

import { multiSelectWidgetLocators } from "../../../../locators/WidgetLocators";
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor: Consolidate imports for better maintainability.

It's commendable to keep all related imports together, but consider consolidating them into fewer lines to enhance readability and maintainability. For example:

- import {
-   agHelper,
-   locators,
-   entityExplorer,
-   deployMode,
-   appSettings,
-   theme,
-   draggableWidgets,
- } from "../../../../support/Objects/ObjectsCore";
+ import { agHelper, locators, entityExplorer, deployMode, appSettings, theme, draggableWidgets } from "../../../../support/Objects/ObjectsCore";

Committable suggestion was skipped due to low confidence.

import {
agHelper,
locators,
Expand All @@ -26,49 +27,19 @@ describe(
agHelper.GetNClick(locators._canvas);
appSettings.OpenAppSettings();
appSettings.GoToThemeSettings();
//Border validation
//cy.contains("Border").click({ force: true });
cy.get(themelocator.border).should("have.length", "3");
cy.borderMouseover(0, "none");
cy.borderMouseover(1, "M");
cy.borderMouseover(2, "L");
cy.get(themelocator.border).eq(1).click({ force: true });
cy.wait("@updateTheme").should(
"have.nested.property",
"response.body.responseMeta.status",
200,
);
cy.wait(1000);
cy.contains("Border").click({ force: true });

//Shadow validation
//cy.contains("Shadow").click({ force: true });
cy.wait(2000);
cy.xpath(theme.locators._boxShadow("L")).click({ force: true });
cy.wait("@updateTheme").should(
"have.nested.property",
"response.body.responseMeta.status",
200,
);
cy.wait(1000);
cy.contains("Shadow").click({ force: true });

//Font
cy.xpath(
"//p[text()='App font']/following-sibling::section//div//input",
).then(($elem) => {
cy.get($elem).click({ force: true });
cy.wait(250);
cy.fixture("fontData").then(function (testdata) {
this.testdata = testdata;
});
agHelper.GetNClick($elem);
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor: Replace XPath with data- attributes for selectors.*

The use of XPath selectors is discouraged due to their fragility and performance issues. It's better to use data-* attributes, which are more robust and maintainable. For instance:

- cy.xpath("//p[text()='App font']/following-sibling::section//div//input")
+ cy.get('[data-cy="app-font-input"]')

Please update all instances where XPath is used to use data-* attributes instead.

Also applies to: 36-42, 55-55


cy.get(themelocator.fontsSelected)
//.eq(10)
agHelper
.GetElement(themelocator.fontsSelected)
.should("contain.text", "Nunito Sans");

cy.get(".rc-virtual-list .rc-select-item-option")
.find(".leading-normal")
agHelper
.GetElement(themelocator.fontOption)
.find(themelocator.fontsSelected)
.eq(3)
.then(($childElem) => {
cy.get($childElem).click({ force: true });
Expand All @@ -80,60 +51,46 @@ describe(
themeFont = `Inter, sans-serif`;
});
});
cy.contains("Font").click({ force: true });

//Color - Bug 23501 - hence skipping
// cy.wait(1000);
// theme.ChangeThemeColor("purple", "Primary");
// cy.get(themelocator.inputColor).should("have.value", "purple");
// cy.wait(1000);
cy.contains("Font").click({ force: true });

// theme.ChangeThemeColor("brown", "Background");
// cy.get(themelocator.inputColor).should("have.value", "brown");
// cy.wait(1000);
// cy.contains("Color").click({ force: true });
appSettings.ClosePane();
});

//Skipping due to mentioned bug
it.skip("2. Publish the App and validate Font across the app + Bug 15007", function () {
it("2. Publish the App and validate Font across the app", function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Re-enable and update the skipped test case.

The test case "2. Publish the App and validate Font across the app" is currently skipped. Given the PR's focus on ensuring font settings are retained correctly, this test case should be re-enabled and updated to reflect the new functionality. Ensure that the test case validates the font consistency across different app components after theme changes.

Also applies to: 62-71

deployMode.DeployApp();
cy.get(".rc-select-selection-item > .rc-select-selection-item-content")
agHelper
.GetElement(
multiSelectWidgetLocators.multiSelectWidgetSelectedOptionContent,
)
.first()
.should("have.css", "font-family", themeFont);
cy.get(".rc-select-selection-item > .rc-select-selection-item-content")
agHelper
.GetElement(
multiSelectWidgetLocators.multiSelectWidgetSelectedOptionContent,
)
.last()
.should("have.css", "font-family", themeFont);
deployMode.NavigateBacktoEditor();
});

it.skip("3. Validate current theme feature", function () {
cy.get("#canvas-selection-0").click({ force: true });
it("3. Apply theme and validate the color", function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize theme application and color validation logic.

The test case "3. Apply theme and validate the color" is well-structured to validate the color consistency after theme application. However, consider using more direct selectors and assertions to streamline the process and improve test performance. For example:

- agHelper.GetNClick(`${themelocator.featuredThemeSection} [data-testid='t--theme-card-Sunrise']`)
+ cy.get('[data-cy="theme-card-Sunrise"]').click()

This change not only simplifies the selector but also enhances the readability and maintainability of the test code.

Also applies to: 80-93

appSettings.OpenAppSettings();
appSettings.GoToThemeSettings();
//Change the Theme
cy.get(commonlocators.changeThemeBtn).click({ force: true });
cy.get(themelocator.currentTheme).click({ force: true });
cy.get(".t--theme-card main > main")
.first()
.invoke("css", "background-color")
.then(() => {
cy.get(".t--draggable-multiselectwidgetv2:contains('more')")
.last()
.invoke("css", "background-color")
.then((selectedBackgroudColor) => {
expect("rgba(0, 0, 0, 0)").to.equal(selectedBackgroudColor);
appSettings.ClosePane();
});
});
agHelper.GetNClick(commonlocators.changeThemeBtn, 0, true);
agHelper.GetNClick(
`${themelocator.featuredThemeSection} [data-testid='t--theme-card-Sunrise']`,
);

//Publish the App and validate change of Theme across the app in publish mode
deployMode.DeployApp();
cy.xpath("//div[@id='root']//section/parent::div").should(
"have.css",
"background-color",
"rgb(165, 42, 42)",
);

agHelper.GetNClick(multiSelectWidgetLocators.multiSelectWidgetTrigger);
agHelper
.GetElement(
multiSelectWidgetLocators.multiSelectWidgetDropdownOptionCheckbox,
)
.first()
.should("have.css", "background-color", "rgb(239, 68, 68)");
});
},
);
6 changes: 4 additions & 2 deletions app/client/cypress/locators/ThemeLocators.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,7 @@
"greenColor": "[style='background-color: rgb(21, 128, 61);']",
"fontsSelected": ".leading-normal",
"currentTheme": ".cursor-pointer:contains('Applied theme')",
"purpleColor": "[style='background-color: rgb(107,114,128);']"
}
"purpleColor": "[style='background-color: rgb(107,114,128);']",
"featuredThemeSection": "[data-testid='t--featured-themes']",
"fontOption": ".rc-virtual-list .rc-select-item-option"
}
6 changes: 6 additions & 0 deletions app/client/cypress/locators/WidgetLocators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,3 +175,9 @@ export const buttongroupwidgetlocators = {
`//*[contains(@class,'bp3-menu-item')]//*[text()='${text}']`,
button: "//*[contains(@class,'t--widget-buttongroupwidget')]//button",
};

export const multiSelectWidgetLocators = {
multiSelectWidgetTrigger: ".t--widget-multiselectwidgetv2 .rc-select-selector",
multiSelectWidgetSelectedOptionContent: ".rc-select-selection-item > .rc-select-selection-item-content",
multiSelectWidgetDropdownOptionCheckbox: ".multi-select-dropdown .rc-select-item-option-selected .bp3-control-indicator"
};
2 changes: 1 addition & 1 deletion app/client/packages/eslint-plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@
"postinstall": "yarn build",
"test:unit": "yarn g:jest"
}
}
}
29 changes: 17 additions & 12 deletions app/client/src/components/propertyControls/ColorPickerComponentV2.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,12 +367,11 @@ const ColorPickerComponent = React.forwardRef(
defaultFullColorPickerValue,
);

const debouncedOnChange = React.useCallback(
debounce((color: string, isUpdatedViaKeyboard: boolean) => {
const debouncedOnChange = useMemo(() => {
return debounce((color: string, isUpdatedViaKeyboard: boolean) => {
props.changeColor(color, isUpdatedViaKeyboard);
}, DEBOUNCE_TIMER),
[],
);
}, DEBOUNCE_TIMER);
}, [props]);

useEffect(() => {
setIsOpen(isOpenProp);
Expand Down Expand Up @@ -545,13 +544,19 @@ const ColorPickerComponent = React.forwardRef(
};
}, [handleKeydown]);

const handleChangeColor = (event: React.ChangeEvent<HTMLInputElement>) => {
const value = event.target.value;
if (isValidColor(value)) {
debouncedOnChange(value, true);
}
setColor(value);
};
const handleChangeColor = useCallback(
(event: React.ChangeEvent<HTMLInputElement>) => {
const value = event.target.value;

if (!value) return;

if (isValidColor(value)) {
debouncedOnChange(value, true);
}
setColor(value);
},
[debouncedOnChange],
);

// if props.color changes and state color is different,
// sets the state color to props color
Expand Down
2 changes: 1 addition & 1 deletion app/client/src/entities/AppTheming/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export interface AppThemeProperties {
colors: {
primaryColor: string;
backgroundColor: string;
[key: string]: string;
[key: Exclude<string, number>]: string;
};
borderRadius: {
[key: string]: string;
Expand Down
1 change: 1 addition & 0 deletions app/client/src/pages/Editor/ThemePropertyPane/ThemeCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ export function ThemeCard(props: ThemeCard) {
"overflow-hidden": !selectable,
"hover:shadow-xl cursor-pointer": selectable,
})}
data-testid={`t--theme-card-${theme.name}`}
onClick={changeSelectedTheme}
>
<MainContainer backgroundColor={backgroundColor}>
Expand Down
4 changes: 2 additions & 2 deletions app/client/src/pages/Editor/ThemePropertyPane/ThemeEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ function ThemeEditor() {

dispatch(updateSelectedAppThemeAction({ applicationId, theme }));
},
[updateSelectedAppThemeAction],
[applicationId, dispatch],
);

/**
Expand All @@ -91,7 +91,7 @@ function ThemeEditor() {
AppThemingMode.APP_THEME_SELECTION,
]),
);
}, [setAppThemingModeStackAction]);
}, [dispatch, themingStack]);

/**
* resets theme
Expand Down
5 changes: 4 additions & 1 deletion app/client/src/pages/Editor/ThemePropertyPane/ThemeSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,10 @@ function ThemeSelector() {
))}
</section>
)}
<section className="relative p-4 space-y-3">
<section
className="relative p-4 space-y-3"
data-testid="t--featured-themes"
>
<Title className="text-sm font-medium">Featured themes</Title>
{systemThemes.map((theme) => (
<ThemeCard
Expand Down
82 changes: 47 additions & 35 deletions app/client/src/pages/Editor/ThemePropertyPane/controls/ThemeColorControl.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { startCase } from "lodash";
import React, { useState } from "react";
import React, { useCallback, useState } from "react";
import styled from "styled-components";

import type { AppTheme } from "entities/AppTheming";
import { Tooltip } from "@appsmith/ads";
import ColorPickerComponent from "components/propertyControls/ColorPickerComponentV2";
import { capitalizeFirstLetter } from "utils/helpers";
import { objectKeys } from "@appsmith/utils";

interface ThemeColorControlProps {
theme: AppTheme;
Expand Down Expand Up @@ -33,50 +34,61 @@ function ThemeColorControl(props: ThemeColorControlProps) {
const [selectedColor, setSelectedColor] = useState<string>("primaryColor");
const [isFullColorPicker, setFullColorPicker] = React.useState(false);

const onColorClick = useCallback(
(e: React.MouseEvent<HTMLDivElement>) => {
const colorName = e.currentTarget.getAttribute("data-color-name");
if (!colorName) return;

setAutoFocus(selectedColor === colorName ? !autoFocus : true);
setSelectedColor(colorName);
},
[autoFocus, selectedColor],
);

const onChangeColor = useCallback(
(color: string) => {
updateTheme({
...theme,
properties: {
...theme.properties,
colors: {
...theme.properties.colors,
[selectedColor]: color,
},
},
});
},
[selectedColor, theme, updateTheme],
);

return (
<div className="space-y-2">
<div className="flex space-x-2">
{Object.keys(theme.properties.colors).map(
(colorName: string, index: number) => {
return (
<Tooltip
content={capitalizeFirstLetter(startCase(colorName))}
key={index}
>
<ColorBox
background={userDefinedColors[colorName]}
className={selectedColor === colorName ? "selected" : ""}
data-testid={`theme-${colorName}`}
onClick={() => {
setAutoFocus(
selectedColor === colorName ? !autoFocus : true,
);
setSelectedColor(colorName);
}}
/>
</Tooltip>
);
},
)}
{objectKeys(theme.properties.colors).map((colorName, index) => {
return (
<Tooltip
content={capitalizeFirstLetter(startCase(colorName as string))}
key={index}
>
<ColorBox
background={userDefinedColors[colorName]}
className={selectedColor === colorName ? "selected" : ""}
data-color-name={colorName}
data-testid={`theme-${colorName}`}
onClick={onColorClick}
/>
</Tooltip>
);
})}
</div>
{selectedColor && (
<div className="pt-1 space-y-1">
<h3>{capitalizeFirstLetter(startCase(selectedColor))}</h3>
<ColorPickerComponent
autoFocus={autoFocus}
changeColor={(color: string) => {
updateTheme({
...theme,
properties: {
...theme.properties,
colors: {
...theme.properties.colors,
[selectedColor]: color,
},
},
});
}}
changeColor={onChangeColor}
color={userDefinedColors[selectedColor]}
data-color={selectedColor}
isFullColorPicker={isFullColorPicker}
isOpen={autoFocus}
key={selectedColor}
Expand Down
Loading