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

refactor: Separate ui rendering from data updates #1692

Merged
merged 12 commits into from Apr 15, 2024
Merged

Conversation

almet
Copy link
Member

@almet almet commented Mar 15, 2024

U.SCHEMA now contains an impacts key, which makes it possible to specify what part of the UI is impacted by data changes.

A new render method has been added on U.Map and U.DataLayer, which is used to rerender the proper parts of the UI depending on the passed properties.

U.FormBuilder calls this render() method (if present), during form changes.

Note

This approach is less specific than a previous one in order to a) not tie the schema to specific callbacks and b) not repeat properties in two different places.

The resulting code is less conservative when considering which parts of the UI are updated when receiving updates, thus rerendering parts of the UI which might not be in need of rerender. The hope is that it won't be a problem, but let's keep our eyes open.

Progress

  • Sort schema keys
  • Add all UI impacts on schema keys
  • Update FormBuilder instanciations to remove callbacks when possible
  • Ensure the tests pass
  • ChoroplethLayer.onEdit is expecting to be passed the builder
  • Use impacts = [] everywhere (remove [''] occurrences)
  • Change how the functional tests are working (see [chore] move umap utils to a module #1718 Remove js tests #1719)
  • Make the JS unit tests pass
  • Make the functional tests pass

@almet almet marked this pull request as ready for review March 15, 2024 15:13
@almet almet force-pushed the schema-impacts branch 2 times, most recently from a2b3ad5 to e817948 Compare March 15, 2024 15:15
@almet almet force-pushed the schema-impacts branch 2 times, most recently from 93d47e2 to 1990cb6 Compare March 18, 2024 10:14
Copy link
Member

@yohanboniface yohanboniface left a comment

Choose a reason for hiding this comment

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

👍

umap/static/umap/js/modules/utils.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.layer.js Show resolved Hide resolved
umap/static/umap/js/modules/schema.js Show resolved Hide resolved
umap/static/umap/js/umap.forms.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.layer.js Show resolved Hide resolved
@almet
Copy link
Member Author

almet commented Apr 2, 2024

This is ready to be merged!

@@ -0,0 +1,185 @@
import re
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mv those tests in test_edit_map.py instead of having a new module ?

@@ -1,397 +1,4 @@
describe('L.Util', function () {
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised to see all those tests removed here ? Should this have been done in that other PR moving utils to a module ? Or is this just missing a rebase ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, it's broken in the current master, and fixed here! 🙈

umap/static/umap/js/umap.layer.js Show resolved Hide resolved
`U.SCHEMA` now contains an `impacts` key, which makes it possible to
specify what part of the UI is impacted by data changes.

A new `render` method has been added on `U.Map` and `U.DataLayer`, which
is used to rerender the proper parts of the UI depending on the passed
properties.

`U.FormBuilder` calls this `render()` method (if present), during form
changes.
@almet almet merged commit 517d3a1 into master Apr 15, 2024
4 checks passed
@almet almet deleted the schema-impacts branch April 15, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants