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

Dynamically load the list of semantic tags #1850

Closed
wants to merge 1 commit into from

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Apr 18, 2023

Depends on openhab/openhab-core#3559 (already merged now)

This is the first time I've ever worked with Vue and modern javascript, so I'd appreciate any tips, thanks!

Adding custom semantic tags is now possible: openhab/openhab-core#3519

This PR loads and and uses the custom tags in the Main UI. As a bonus, the hard coded semantic tags + all the translations are removed from mainui, so they no longer need to be maintained separately. They are now loaded from core through the REST API.

I could use some help in testing this PR because I am not familiar with everything that may be affected. To test, install this jar file from here: https://github.com/jimtng/openhab-webui/releases/tag/semantics-0.0.1 onto the latest snapshot

Even if you don't have any custom semantics, at least test to make sure that everywhere the semantic tags are used, work as expected. I am not entirely familiar with the MainUI especially around its page builder.

image

image

@jimtng jimtng requested a review from a team as a code owner April 18, 2023 12:16
@jimtng jimtng force-pushed the dynamic-semantics branch from 6146f1b to 2fea244 Compare April 18, 2023 12:18
@relativeci
Copy link

relativeci bot commented Apr 18, 2023

Job #959: Bundle Size — 15.64MiB (-0.66%).

669c402(current) vs 4faf2ba main#958(baseline)

⚠️ Bundle contains 16 duplicate packages

Metrics (4 changes)
                 Current
Job #959
     Baseline
Job #958
Initial JS 1.66MiB(-3.92%) 1.73MiB
Initial CSS 608.85KiB(~-0.01%) 608.87KiB
Cache Invalidation 93.94% 93.94%
Chunks 219 219
Assets 689 689
Modules 1689(-1.69%) 1718
Duplicate Modules 85 85
Duplicate Code 1.74%(+0.58%) 1.73%
Packages 137 137
Duplicate Packages 15 15
Total size by type (3 changes)
                 Current
Job #959
     Baseline
Job #958
CSS 858.58KiB (~-0.01%) 858.61KiB
Fonts 526.1KiB 526.1KiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.17MiB (-0.73%) 9.24MiB
Media 295.6KiB 295.6KiB
Other 4.69MiB (-0.75%) 4.73MiB

View job #959 reportView main branch activity

@jimtng jimtng force-pushed the dynamic-semantics branch from 2fea244 to 8c9d179 Compare April 18, 2023 12:28
@jimtng jimtng force-pushed the dynamic-semantics branch 2 times, most recently from cca6481 to 9af96d6 Compare April 20, 2023 20:27
@jimtng
Copy link
Contributor Author

jimtng commented Apr 20, 2023

Rebased

@florian-h05
Copy link
Contributor

@jimtng Can you please rebase again?

Before starting to review this, I'd like to get @ghys opinion regarding your taken approach because I am not sure if this is the way to go or if using a VueX store would be a better solution.

@jimtng jimtng force-pushed the dynamic-semantics branch from 9af96d6 to dcd2b7b Compare May 6, 2023 21:44
@jimtng
Copy link
Contributor Author

jimtng commented May 6, 2023

Rebased. If there's a better way, I'll be happy to close this. I really don't know anything about Vue.

@ghys
Copy link
Member

ghys commented May 7, 2023

@jimtng
Copy link
Contributor Author

jimtng commented May 8, 2023

  • if the semantic tags are now fetched dynamically, then architecturally-wise they don't belong in the assets folder anymore, theoretically that is for non-dynamic things bundled with the UI code; it would be better to make it a service of sorts, perhaps in the js/openhab folder.

I agree with this and I suppose this would be a trivial change.

  • as @florian-h05 says making it part of the Vuex store is best if it's ultimately user-controllable

This part I have zero knowledge of. Is there an example of other things that have implemented Vuex store? I can only find // TODO use a Vuex store scattered here and there.

@florian-h05
Copy link
Contributor

Unfortunately I haven’t much experience with Vuex either, but the Vuex Stores are defined here:

https://github.com/openhab/openhab-webui/tree/main/bundles/org.openhab.ui/web/src/js/store

You can access the store with this.$store.

@jimtng
Copy link
Contributor Author

jimtng commented May 8, 2023

Unfortunately I haven’t much experience with Vuex either, but the Vuex Stores are defined here:

https://github.com/openhab/openhab-webui/tree/main/bundles/org.openhab.ui/web/src/js/store

You can access the store with this.$store.

Yes, I've already found that, but nothing has been implemented using it to model from.

@florian-h05
Copy link
Contributor

@jimtng I am currently didding into Vuex, I think I can open a PR soon.

@florian-h05
Copy link
Contributor

@jimtng
PR is now open, see #1882. Please check if it works with your use case as expected, and sorry for rushing but I though before I start looking into Vuex and explaining it to you, it is faster and more efficient to write the code myself.

@jimtng
Copy link
Contributor Author

jimtng commented May 8, 2023

Thank you! I'll close this PR.

@jimtng jimtng closed this May 8, 2023
ghys pushed a commit that referenced this pull request Jun 28, 2023
)

Supersedes #1850.
Closes #1822.

Depends on openhab/openhab-core#3559 (already
merged now).
Adding custom semantic tags is now possible:
openhab/openhab-core#3519.

This PR loads the Semantic tags when MainUI is loaded the first time and
stores them in Vuex.
This allows the removal of the hard-coded Semantic tags and the
translations from the assets and therefore makes the initial JS smaller.

--
Signed-off-by: Florian Hotze <[email protected]>
@jimtng jimtng deleted the dynamic-semantics branch September 12, 2023 10:07
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.

3 participants