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.

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

fix: Upgrade map widget library to support react 17 #19315

Merged
merged 18 commits into from Jan 13, 2023

Conversation

jsartisan
Copy link
Contributor

@jsartisan jsartisan commented Dec 30, 2022

This PR:

  • updates the react 16 to react 17
  • replaces the underlying library for the map widget
  • adds clustering of markers
  • refactor code for map widget's component

Description

Fixes #16946

Type of change

  • Breaking change

How Has This Been Tested?

  • Manually

Test Plan

https://github.com/appsmithorg/TestSmith/issues/2149

Issues raised during DP testing

  1. fix: Upgrade map widget library to support react 17 #19315 (comment)

Checklist:

Dev activity

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • PR is being merged under a feature flag

QA activity:

  • Test plan has been approved by relevant developers
  • Test plan has been peer reviewed by QA
  • Cypress test cases have been added and approved by either SDET or manual QA
  • Organized project review call with relevant stakeholders after Round 1/2 of QA
  • Added Test Plan Approved label after reveiwing all Cypress test

@vercel
Copy link

vercel bot commented Dec 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview Jan 13, 2023 at 1:29PM (UTC)

@github-actions github-actions bot added App Viewers Pod This label assigns issues to the app viewers pod Enhancement New feature or request High This issue blocks a user from building or impacts a lot of users Map Widget Bug Something isn't working and removed Enhancement New feature or request labels Dec 30, 2022
@github-actions github-actions bot added the Enhancement New feature or request label Dec 30, 2022
@github-actions github-actions bot removed the Enhancement New feature or request label Dec 30, 2022
@github-actions github-actions bot added Enhancement New feature or request and removed Enhancement New feature or request labels Dec 30, 2022
@jsartisan
Copy link
Contributor Author

/ok-to-test sha=218c9f7

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3807397999.
Workflow: Appsmith External Integration Test Workflow.
Commit: 218c9f7.
PR: 19315.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=19315&runId=3807397999_1

@github-actions
Copy link

The following are new failures, please fix them before merging the PR cypress/integration/Smoke_TestSuite/ClientSideTests/Onboarding/GuidedTour_spec.js
cypress/integration/Smoke_TestSuite/ClientSideTests/Templates/Fork_Template_To_App_spec.js
cypress/integration/Smoke_TestSuite/ServerSideTests/GenerateCRUD/Postgres2_Spec.ts
cypress/integration/Smoke_TestSuite/ServerSideTests/JsFunctionExecution/JSFunctionExecution_spec.ts
cypress/integration/Smoke_TestSuite/ServerSideTests/QueryPane/EmptyDataSource_spec.js

@github-actions
Copy link

The following are new failures, please fix them before merging the PR cypress/integration/Smoke_TestSuite/ServerSideTests/JsFunctionExecution/JSFunctionExecution_spec.ts
cypress/integration/Smoke_TestSuite/ServerSideTests/QueryPane/EmptyDataSource_spec.js
cypress/integration/Smoke_TestSuite/ServerSideTests/QueryPane/EmptyDataSource_spec.js

Copy link
Contributor

@keyurparalkar keyurparalkar left a comment

Choose a reason for hiding this comment

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

Is this issue occuring: Component re-renders on change in zoom level?

app/client/src/widgets/MapWidget/component/Map.tsx Outdated Show resolved Hide resolved
id="map"
ref={mapRef}
/>
<Clusterer map={map} markers={markers} {...rest} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we require the clustering feature in this PR?

<Clusterer map={map} markers={markers} {...rest} />
{React.Children.map(children, (child) => {
if (React.isValidElement(child)) {
return React.cloneElement(child as React.ReactElement<any>, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using cloneElement to pass down map object to all children?

useEffect(() => {
if (!searchBoxRef.current) return;

searchBoxObjRef.current = new window.google.maps.places.SearchBox(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check that window has google and map object

@@ -304,7 +304,7 @@ class MapWidget extends BaseWidget<MapWidgetProps, WidgetState> {
};
}

updateCenter = (lat: number, long: number, title?: string) => {
updateCenter = (lat?: number, long?: number, title?: string) => {
this.props.updateWidgetMetaProperty("center", { lat, long, title });
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we save lng or long in the DSL?

onSearchBoxMounted = (ref: SearchBox) => {
this.searchBox = ref;
onSearchBoxMounted = (ref: any) => {
if (window) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also check if map object exists?

case Status.FAILURE:
return <span>Error in the component</span>;
case Status.SUCCESS:
return <span>Component loaded....</span>;
Copy link
Contributor

Choose a reason for hiding this comment

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

When there is a FAILURE or LOADING state are we should these span elements or appsmith defaults?

@laveena-en
Copy link
Contributor

@jsartisan A loading state is seen in place of the text box under Initial location.

And the corresponding location isn’t showing up on the map, on searching for a particular city.
Can you please check? https://ivp6ez0ljb.vmaker.com/record/8XtxRhdwQYClpbub

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3854745148.
Workflow: Appsmith External Integration Test Workflow.
Commit: 151a37e.
PR: 19315.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=19315&runId=3854745148_1

@yaldram
Copy link
Contributor

yaldram commented Jan 6, 2023

We still can see this issue in the modal widget, Drag to select multiple widgets is not working. @jsartisan please check this with @rahulramesha, it is related to the onClickCapture breaking change in react 17.

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

The following are new failures, please fix them before merging the PR cypress/integration/Smoke_TestSuite/ClientSideTests/BugTests/Autocomplete_JS_spec.ts

@yaldram
Copy link
Contributor

yaldram commented Jan 9, 2023

QA - The drag to select multiple widgets inside the Modal widget is fixed by Ashok, and is working as expected.

@kamakshibhat-appsmith
Copy link

kamakshibhat-appsmith commented Jan 11, 2023

Issues and observations: @yaldram

  1. Issue with footer remix icons not showing up
  2. the clickable action for open up the Errors/logs pane is not working LOOM

@github-actions github-actions bot removed the Enhancement New feature or request label Jan 11, 2023
@github-actions github-actions bot added the Enhancement New feature or request label Jan 11, 2023
@github-actions github-actions bot added Enhancement New feature or request and removed Enhancement New feature or request labels Jan 11, 2023
@yaldram
Copy link
Contributor

yaldram commented Jan 13, 2023

/ok-to-test sha=f14aa50

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3911918276.
Workflow: Appsmith External Integration Test Workflow.
Commit: f14aa50.
PR: 19315.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=19315&runId=3911918276_1

@github-actions
Copy link

The following are new failures, please fix them before merging the PR cypress/integration/Smoke_TestSuite/ClientSideTests/Git/GitSync/RepoLimitExceededErrorModal_spec.js
cypress/integration/Smoke_TestSuite/ClientSideTests/Templates/Fork_Template_To_App_spec.js
cypress/integration/Smoke_TestSuite/ClientSideTests/VisualTests/WidgetsLayout_spec.js

@rohitagarwal88 rohitagarwal88 merged commit eba4745 into release Jan 13, 2023
@rohitagarwal88 rohitagarwal88 deleted the chore/map-widget-lib-change branch January 13, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Viewers Pod This label assigns issues to the app viewers pod Bug Something isn't working Enhancement New feature or request High This issue blocks a user from building or impacts a lot of users Map Widget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade map widget library to support react 17
8 participants