-
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
refactor: JSONForm eliminate the use of canvasWidgets and remove childStylesheet from dynamicBindingPathList #18715
Conversation
…dStylesheet from dynamicBindingPathList
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
/ok-to-test sha=064f810 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3627534023. |
@@ -612,3 +613,31 @@ export const addPropertyToDynamicPropertyPathList = ( | |||
]; | |||
} | |||
}; | |||
|
|||
export const migrateChildStylesheetFromDynamicBindingPathList = ( | |||
currentDSL: DSLWidget, |
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.
Could you have a look at this utility we wrote to simplify the migration codes traverseDSLAndMigrate
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.
Ohh nice, I was looking for something like this.
/ok-to-test sha=2d09a9d |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3635935335. |
/ok-to-test sha=0001842 |
/ok-to-test sha=0001842 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3638307458. |
/ok-to-test sha=37d454f |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3645779329. |
/ok-to-test sha=d4e6115 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3648067597. |
/ok-to-test sha=7cedc8c |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3654223858. |
Tested this PR and LGTM
|
Description
TL;DR
This refactors some of the code for JSONForm widget, which helps it work along with the upcoming List v2.0. It also contains some changes to theming's internal working which may improve some performance (very minor) but it originally spawned because of the first refactor.
This PR comprises 2 refactors.
The JSONForm used
canvasWidgets
directly to get the unevalschema
and unevalchildStylesheet
as it used both to generate the next schema.How are both the problems eliminated:
uneval
schema
Instead of taking the old schema and overwriting the modified/added/deleted bits to it, the schema parser now only generates the first schema if not present and post that the parser provides paths of the properties that got deleted and the modifed/added paths with value. It still returns the amended schema using the prev but it's not used anymore. The paths that are returned are then directly used to update using the
batchUpdateWidgetProperty
. We realised that when something is modified or added to the source data, fresh config is always generated to old config was not in play so uneval old schema was not need anymore.uneval
childStylesheet
The reason for getting evaluated
childStylesheet
was when a new widget was dropped, the paths ofchildStylesheet
was automatically added the thedynamicBindingPathList
. This lead to the the property to come always in the evaluated form.This was fixed along with a migration that removes all the widgets where the paths are added to the dynamicBindingPathList.
Fixes #16616
Note to QA
This PR does not contain any bug/feature and should not introduce any regression .
Type of change
How Has This Been Tested?
Test Plan
Issues raised during DP testing
Checklist:
Dev activity
QA activity: