-
Notifications
You must be signed in to change notification settings - Fork 809
Global Contexts #7474
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
base: main
Are you sure you want to change the base?
Global Contexts #7474
Conversation
β¦tup, examples of how to create a context and how to consume a context.
β¦tup, examples of how to create a context and how to consume a context.
β¦texts # Conflicts: # 16/umbraco-cms/customizing/extending-overview/extension-types/global-context.md
return this.#preferences.get(key); | ||
} | ||
|
||
getHostElement(): Element { |
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.
getHostElement()
is required by UmbContextMinimal, but I'm not sure what the intent of this is? To return a DOM element? The DOM element that the context is attached to? That's kind of how it seems to be getting used in the Umbraco backoffice samples I've found, but it seems odd.
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.
Agreed, it seems odd. We need @nielslyngsoe's perspective on this. We could provide any value as a context, including simple types such as string, number, and array, I thought.
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.
In my Umbraco project I'm using for testing all of this docs code, I just did return new Element();
inside of there to keep the project compiling, but I feel dirty publishing that without further comment or without understanding what might blow up.
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.
Hi everyone :-)
The UmbContextMinimal abstraction/interface might confuse matters, so for understanding, I would say ignore that one. It only exists to avoid circular dependencies and separation of concerns.
So the particular need for the getHostElement
is that this is the only part of a UmbControllerHost
that the Context implementation must have.
But it could have been simpler if the example interface extended UmbControllerHost
. But for someone not basing their class on UmbContextBase(Which is an extension of UmbControllerBase) will then have a more complex interface to adhere to β so that could be the downside to it, but maybe only relevant for complex use cases, like someone implementing a different framework, so maybe an aspect to ignore.
So you could avoid declaring the getHostElement by making the interface extend UmbControllerHost β this is all handled, from an implementation perspective, as your class extends UmbContextBase.
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.
... and I may add that the getHostElement
is supposed to return the element that hosts the controllers. For a Context Consumer or Provider, this is the element used to fire or listen for DOM Events. So this cannot be a virtual node, it needs to be the Element that the code belongs to. But @bszyman maybe in your case it was not needed, so it worked from a compiler perspective. But it would eventually result in things not working. :-)
π Description
Revised Global Contexts section for clarity and added best practices guidance.
π Related Issues (if applicable)
#7430
β Contributor Checklist
I've followed the Umbraco Documentation Style Guide and can confirm that:
Product & Version (if relevant)
v16
Deadline (if relevant)
N/A
π Helpful Resources