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: widget icon in Connect To options in oneclick binding menu #36703

Merged
merged 6 commits into from
Oct 7, 2024
Merged

fix: widget icon in Connect To options in oneclick binding menu #36703

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
dbaedc8
get widgetConfig for iconSVG
jacquesikot Sep 24, 2024
216ec68
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
rahulbarwal Sep 25, 2024
5321f5d
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
rahulbarwal Oct 4, 2024
41d6913
refactor: Update useConnectToOptions to improve widget icon handling
rahulbarwal Oct 4, 2024
1ed4915
refactor: Improve widget icon handling in useDropdown
rahulbarwal Oct 4, 2024
80a6355
refactor: Improve widget icon handling in OneClickBinding spec_utilit…
rahulbarwal Oct 4, 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
17 changes: 16 additions & 1 deletion app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/spec_utility.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,22 @@ export class OneClickBinding {
column: Record<string, string> = {},
) {
agHelper.GetNClick(oneClickBindingLocator.datasourceDropdownSelector);

agHelper.GetElement("[role='menu']").then(($menu) => {
if (
$menu.find(oneClickBindingLocator.datasourceQuerySelector()).length > 0
) {
cy.wrap($menu)
.find(oneClickBindingLocator.datasourceQuerySelector())
.should("have.length.greaterThan", 0)
.each(($item) => {
cy.wrap($item)
.find("img")
.should(($img) => {
expect($img).to.have.attr("src").and.not.be.empty;
});
});
}
});
Comment on lines +15 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Class, let's review this new code block and make some improvements!

While the addition of image validation for datasource items is a good step, there are a few areas where we can enhance our code:

  1. Instead of using cy.wrap directly, let's create a custom command or helper method for better maintainability and reusability.

  2. We should avoid using hardcoded waits like agHelper.Sleep(3000). Instead, let's use Cypress's built-in waiting mechanisms or create custom waiting functions.

  3. This method is becoming quite long. Consider breaking it down into smaller, more focused methods for better readability and maintainability.

Here's an example of how we could refactor this part:

private assertDatasourceImages() {
  return agHelper.GetElement("[role='menu']").then(($menu) => {
    if ($menu.find(oneClickBindingLocator.datasourceQuerySelector()).length > 0) {
      cy.wrap($menu)
        .find(oneClickBindingLocator.datasourceQuerySelector())
        .should("have.length.greaterThan", 0)
        .each(($item) => {
          cy.wrap($item)
            .find("img")
            .should("have.attr", "src")
            .and("not.be.empty");
        });
    }
  });
}

Then, in the ChooseAndAssertForm method:

this.assertDatasourceImages();

Remember, class, breaking down complex methods into smaller, more focused ones is like solving a big problem by tackling smaller parts one at a time. It makes our code easier to understand and maintain!

expandLoadMoreOptions();

agHelper.AssertElementAbsence(oneClickBindingLocator.connectData);
Expand Down
5 changes: 4 additions & 1 deletion ...getQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ function useConnectToOptions(props: ConnectToOptionsProps) {
// This is the path we bind to the sourceData field Ex: `{{Table1.selectedRow}}`
const { widgetBindPath } =
getOneClickBindingConnectableWidgetConfig(currWidget);
const iconSVG =
WidgetFactory.getConfig(currWidget.type)?.iconSVG ||
currWidget.iconSVG;

return {
id: widgetId,
Expand All @@ -250,7 +253,7 @@ function useConnectToOptions(props: ConnectToOptionsProps) {
<DatasourceImage
alt="widget-icon"
className="dataSourceImage"
src={currWidget.iconSVG}
src={iconSVG}
/>
</ImageWrapper>
),
Expand Down
5 changes: 4 additions & 1 deletion ...idgetQueryGeneratorForm/WidgetSpecificControls/OtherFields/Field/Dropdown/useDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,15 @@ export function useDropdown(props: OneClickDropdownFieldProps) {
if (getOneClickBindingConnectableWidgetConfig) {
const { message, widgetBindPath } =
getOneClickBindingConnectableWidgetConfig(currWidget);
const iconSVG =
WidgetFactory.getConfig(currWidget.type)?.iconSVG ||
currWidget.iconSVG;

return {
id: currWidgetId,
value: widgetBindPath,
label: currWidget.widgetName,
icon: <StyledImage alt="widget-icon" src={currWidget.iconSVG} />,
icon: <StyledImage alt="widget-icon" src={iconSVG} />,
data: {
widgetType: currWidget.type,
widgetName: currWidget.widgetName,
Expand Down
Loading