-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
Add oh-context component #2533
Add oh-context component #2533
Conversation
#1995 Bundle Size — 10.63MiB (+0.05%).Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
Current #1995 |
Baseline #1991 |
|
---|---|---|
Initial JS | 1.87MiB (+0.29% ) |
1.86MiB |
Initial CSS | 607.87KiB |
607.87KiB |
Cache Invalidation | 18.65% |
19.42% |
Chunks | 223 |
223 |
Assets | 246 |
246 |
Modules | 2888 (+0.14% ) |
2884 |
Duplicate Modules | 149 |
149 |
Duplicate Code | 1.85% |
1.85% |
Packages | 97 |
97 |
Duplicate Packages | 2 |
2 |
Bundle size by type 1 change
1 regression
Current #1995 |
Baseline #1991 |
|
---|---|---|
JS | 8.82MiB (+0.06% ) |
8.81MiB |
CSS | 891.48KiB |
891.48KiB |
Fonts | 526.1KiB |
526.1KiB |
Media | 295.6KiB |
295.6KiB |
IMG | 140.74KiB |
140.74KiB |
HTML | 1.24KiB |
1.24KiB |
Other | 871B |
871B |
Bundle analysis report Branch JustinGeorgi:jag-oh-context Project dashboard
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.
LGTM, thanks!
Very nice addition.
It took me a bit to figure out the scoping of the vars, so I added JSDoc to the getVariableScope
method:
/**
* Get the oh-context variables scope for a given variable key.
*
* If no variable with the given key is found in the given scope, the parent context/scope is checked, and so on.
* If the variable is not found in any scope, <code>null</code> is returned.
*
* oh-context variables are local in scope to the oh-context and it's children and take precedence over other variables
* of the same name from higher contexts/scopes, including normal variables.
* Changes to oh-context variables done by children are propagated to the parent, which is not the case with normal variables.
*
* @param {object} varObj the object containing the variables for each context/scope
* @param {string} scopeObj the key of the given variable context/scope
* @param {string} key the key of the variable
* @returns {string|null} the key of the variable context/scope to be used
*/
Please let me know it that doc is correct @JustinGeorgi.
I also did several minor code improvements, I will soon rebase and resolve the conflicts.
Signed-off-by: Justin Georgi <justin.georgi@gmail.com>
Signed-off-by: Justin Georgi <justin.georgi@gmail.com>
Signed-off-by: Justin Georgi <justin.georgi@gmail.com>
Signed-off-by: Justin Georgi <justin.georgi@gmail.com>
Signed-off-by: Justin Georgi <justin.georgi@gmail.com>
Signed-off-by: Justin Georgi <justin.georgi@gmail.com>
Signed-off-by: Justin Georgi <justin.georgi@gmail.com>
Signed-off-by: Justin Georgi <justin.georgi@gmail.com>
Signed-off-by: Justin Georgi <justin.georgi@gmail.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
bd850fa
to
e060264
Compare
@JustinGeorgi As far as I have tested with your demo widgets, nothing got broken by the rebasing, even though the conflict was a bit tricky. Please have a final look/do a final test and let me know when I should merge. |
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Thanks, the additions look pretty good to me. I don't know if it's worth being more precise on the last line of the JSDoc because the normal variable system for propagating variable values to parents is actually different for components and widgets (components do, widgets do not). So technically,
is a little mis-leading about the normal variables, but I personally don't think it's a big concern because it is true that both work with the oh-context.
Checks out with all the tests I've run. As far as I'm concerned it's ready to merge. |
Thanks for the feedback.
|
Yeah, that's great. |
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Can you please update the docs? |
Of course. I'll probably have time in the next week or so. |
This is excellent news! Thank-you for all the hard work on this. This will greatly simplify the widgets I have been working on recently. Can you tell me - how can I test this? |
Basically you use oh-context „above“ all components that should access its context, so I guess in most cases as the root component of your widget. Add the actual widget into the default slots of the oh-context component and you‘ll be able to access constants on the For more detailed explanation please wait for the docs. |
Thanks Florian Your example widget above looks pretty self-explanantory. I noticed the docs are not updated yet and I presume it's not in the current stable distribution but I was wondering whether it is in the 4.2 M3 milestone? Thanks again for the work on this. It promises to be awesome. I've got some widgets to re-write having spent a lot of time scratching my head and working around the lack of declared variables with defaults or passing variables into custom sub-widgets. The functions willbe useful too. |
FYI this PR is @JustinGeorgi ‘s work, not mine. I am the one who reviewed and merged, but not who wrote it.
Once Justin created a PR to update the docs, they will be at https://next.openhab.org/docs. https://www.openhab.org/docs holds the stable docs, next always the latest docs from the current state of the repo.
No, if it was, the release notes would mention it, and as M3 was released two weeks ago and this PR was merged yesterday, it obviously cannot be in the milestone 😉 |
|
Closes #2437, Closes #2148
This PR adds a new component, the oh-context. Similar to the repeater, this component is not rendered, but injects information into the widget at it's tree location.
The component allows for that addition of three things:
Here is are widgets that I have been using to test all these different capabilities:
Main widget:
Sub widget included in the main widget:
I've done some pretty extensive testing, but this has grown into a fairly large addition, so any and all additional testing is appreciated. There's no telling what I've missed.