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

Reactivity #178

Merged
merged 19 commits into from
Jul 19, 2024
Merged

Reactivity #178

merged 19 commits into from
Jul 19, 2024

Conversation

jgonggrijp
Copy link
Contributor

This branch closes #177 and also partially addresses #123, #130 and #131.

These changes demonstrate a few tricks and principles (apart from a few other, minor cleanups):

  • View composition. Nested views are owned by their enveloping views, rather than being injected from the outside by unrelated code.
  • Views decide for themselves when to render.
  • Sharing models and collections between views whenever possible.
  • Long distance communication between views through events in shared models, rather than through global state. For example, each entry in the catalog dropdown and the collection dropdown is now a separate view that is aware of the catalog/collection it represents. It will update its active class in response to focus/blur events from its .model (i.e., the catalog or collection in question). This avoids having to redraw both dropdown menus entirely every time the selection changes.
  • Using enter and exit events on a state model, to insert a new view into the DOM and also cleanly remove it again when appropriate. This is also used to trigger the focus/blur events on the catalogs and collections.
  • Using route events from a router, rather than defining route-handling methods directly within the router itself.
  • Loose coupling through reactivity (also known as the law of Demeter): individual model and view classes know relatively little about each other, as can be seen from the reduced usage of GlobalVariables. The main.js creates instances of these classes and adds coupling from the outside, mostly by binding methods of one instance to events of another.

I write "demonstrate", because there is also still some code that could use the above treatment, but which I skipped. That means that #123, #130 and #131 cannot be closed yet.

You are cordially invited to read the code changes for inspiration. I expect at least some of the changes to raise questions; please do not hesitate to ask for clarification where needed.

I expect these changes to conflict with changes that you have written in parallel. I propose to merge your changes first, I will rebase my changes on yours after that.

@jgonggrijp jgonggrijp mentioned this pull request Jul 11, 2024
Copy link
Contributor

@tijmenbaarda tijmenbaarda left a comment

Choose a reason for hiding this comment

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

Great!

I must admit that it took me some time to understand how this works. Part of that was because it is hard to find out what kind of values are expected in the various properties of the navigationState model. Is there a usual way to document what the model's properties mean?

I think also collection.search.view.js has to be renamed to catalog.search.view.js, but maybe it is better to do that after #183 is merged.

@jgonggrijp
Copy link
Contributor Author

I must admit that it took me some time to understand how this works.

No shame in that, especially given that it was already expected.

Part of that was because it is hard to find out what kind of values are expected in the various properties of the navigationState model. Is there a usual way to document what the model's properties mean?

Not that I'm aware of, but we can probably agree on something. Do you have suggestions?

I think also collection.search.view.js has to be renamed to catalog.search.view.js, but maybe it is better to do that after #183 is merged.

Agreed!

@tijmenbaarda
Copy link
Contributor

@jgonggrijp : I have merged develop into your branch so that I can continue with the next step. The merge conflicts were not dramatic and it helped me to understand your new set-up better.

@jgonggrijp
Copy link
Contributor Author

@tijmenbaarda I will rebase this branch on develop before merging, because the history will be cleaner in that way. However, it is no problem if you already based new work on this branch; you can rebase your new branch on develop again afterwards.

jgonggrijp and others added 19 commits July 19, 2024 22:58
Because that is the version that runs on our servers.
This is not entirely right yet. The "load more" button currently still
relies on there being a global search view.
Merging the collections simplified the integration between the search
view and the records list view.
@jgonggrijp jgonggrijp force-pushed the feature/reactivity branch from e76d7b4 to 7379d04 Compare July 19, 2024 21:46
@jgonggrijp jgonggrijp merged commit 5b9308d into develop Jul 19, 2024
@jgonggrijp jgonggrijp deleted the feature/reactivity branch July 19, 2024 21:48
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.

Use a model to track the currently selected component
2 participants