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

Sitemap Editor: Buttongrid support #2193

Merged
merged 7 commits into from
Dec 13, 2023
Merged

Sitemap Editor: Buttongrid support #2193

merged 7 commits into from
Dec 13, 2023

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Dec 1, 2023

This PR adds the Buttongrid element to the sitemap editor.
There is also some cleanup for configuration on small screens.
See openhab/openhab-core#3810 and openhab/openhab-android#3494

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege requested a review from a team as a code owner December 1, 2023 17:21
Copy link

relativeci bot commented Dec 1, 2023

Job #1298: Bundle Size — 15.82MiB (+0.39%).

a5ff0ef(current) vs b7270d2 main#1158(baseline)

Important

Bundle introduced 1 and removed 1 duplicate package – View changed duplicate packages

Warning

Bundle introduced 13 new packages: @jsep-plugin/regex, @jsep-plugin/arrow, @jsep-plugin/object and 10 more – View changed packages

Bundle metrics  Change 10 changes Regression 5 regressions Improvement 1 improvement
                 Current
Job #1298
     Baseline
Job #1158
Regression  Initial JS 1.9MiB(+13.5%) 1.67MiB
Regression  Initial CSS 609.63KiB(+0.12%) 608.89KiB
Change  Cache Invalidation 93.81% 93.95%
Change  Chunks 217(-0.91%) 219
Change  Assets 683(-0.87%) 689
Change  Modules 3029(+78.18%) 1700
Regression  Duplicate Modules 170(+88.89%) 90
Improvement  Duplicate Code 1.6%(-17.95%) 1.95%
Regression  Packages 152(+10.14%) 138
Regression  Duplicate Packages 16(+6.67%) 15
Bundle size by type  Change 3 changes Regression 3 regressions
                 Current
Job #1298
     Baseline
Job #1158
Regression  JS 9.29MiB (+0.43%) 9.25MiB
Regression  Other 4.75MiB (+0.42%) 4.73MiB
Regression  CSS 860.95KiB (+0.17%) 859.49KiB
Not changed  Fonts 526.1KiB 526.1KiB
Not changed  Media 295.6KiB 295.6KiB
Not changed  IMG 140.74KiB 140.74KiB
Not changed  HTML 1.23KiB 1.23KiB

View job #1298 reportView mherwege:buttongrid branch activity

@mherwege
Copy link
Contributor Author

mherwege commented Dec 1, 2023

@lolodomo I hope I have the JSON format right, but would need a working UI to test. I understand there is none yet. I will correct if there is an issue.

@lolodomo
Copy link
Contributor

lolodomo commented Dec 2, 2023

I started to implement buttongrid widget. I will publish a WIP PR today so that you can test your PR.

@lolodomo
Copy link
Contributor

lolodomo commented Dec 2, 2023

@mherwege : you can try with BasicUI built from PR #2195

@mherwege mherwege marked this pull request as draft December 3, 2023 09:56
@lolodomo
Copy link
Contributor

lolodomo commented Dec 3, 2023

@mherwege : look at my message, I propose to change the syntax: openhab/openhab-core#3441 (comment)
WDYT ?

@mherwege
Copy link
Contributor Author

mherwege commented Dec 3, 2023

@lolodomo I am fine with that change. If it is agreed, I will adapt the sitemap editor PR accordingly.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege marked this pull request as ready for review December 5, 2023 11:03
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege
Copy link
Contributor Author

mherwege commented Dec 5, 2023

@lolodomo I now adapted it to the syntax with colons to separate row and column (row:column:command).

Here are few screenshots:

image image image image

@mherwege mherwege changed the title Sitemap Editor: buttongrid support Sitemap Editor: Buttongrid support Dec 5, 2023
@florian-h05
Copy link
Contributor

Is this finished and ready for review?

@mherwege
Copy link
Contributor Author

mherwege commented Dec 6, 2023

Is this finished and ready for review?

@florian-h05 Yes it is.

@lolodomo
Copy link
Contributor

lolodomo commented Dec 6, 2023

Is it compatible with the last syntax (PR merged yesterday) ?

@mherwege
Copy link
Contributor Author

mherwege commented Dec 6, 2023

Is it compatible with the last syntax (PR merged yesterday) ?

@lolodomo I now adapted it to the syntax with colons to separate row and column (row:column:command).

If this is the syntax, yes.

@ghys ghys added rebuild trigger a new Jenkins job enhancement New feature or request main ui Main UI labels Dec 8, 2023
@ghys ghys added this to the 4.1 milestone Dec 8, 2023
Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Thanks @mherwege it seems to work well.

I think the header/tabs in the bottom pane (Widget, Visibility...) when on some phones with the iOS theme is becoming quite crowded, like this:

image

We might need to find a solution design-wise for these, when there are too many 'tabs' to display and there isn't enough room.

Otherwise LGTM!

@mherwege
Copy link
Contributor Author

mherwege commented Dec 8, 2023

@ghys I see. It might be better to put Visibility at the end cor this. I put it there because I found that the more logical position.

I did create these extra tabs because content inside one tab became too much.

Is there a way to swipe the tabs slightly left/right? That might be a solution.

@ghys
Copy link
Member

ghys commented Dec 8, 2023

I think normally the tabs can be scrolled but in this case (I tried on my Android) they don't. So you have something like this but no way to access what's not visible in the header:

Screenshot_20231208_202934.jpg

I'll try to have a look!

<f7-icon ios="f7:plus" md="material:add" aurora="f7:plus" />
<f7-icon ios="f7:multiply" md="material:close" aurora="f7:multiply" />
</f7-fab>

<f7-sheet class="sitemap-details-sheet" :backdrop="false" :close-on-escape="true" :opened="detailsOpened" @sheet:closed="detailsOpened = false">
<f7-sheet v-if="currentTab === 'tree'" class="sitemap-details-sheet" :backdrop="false" :close-on-escape="true" :opened="detailsOpened" @sheet:closed="detailsOpened = false">
<f7-page>
<f7-toolbar tabbar bottom>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add ‘scrollable’ here may do.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege
Copy link
Contributor Author

mherwege commented Dec 9, 2023

@ghys I added a scrollable attribute to the tabbar. I hope that makes it better.

@florian-h05
Copy link
Contributor

I have tested this now on my iPhone 14 and in the Chrome DevTools with "Pixel 7" setting, and in both cases, the tab bar is now scrollable.
@ghys Please let me know if I can merge this or should wait for you.

@lolodomo
Copy link
Contributor

The new widget is now implemented (merged) in Basic UI so we really need this PR before OH 4.1 code is frozen ;)

@florian-h05
Copy link
Contributor

Sure. Let's wait for Yannick till Friday evening, and then merge,

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

I'm happy with this fix, maybe we should move the chevron (to collapse) to the left so we don't have to scroll to find it, but it'll do for now.
Thanks for addressing my concerns!

@ghys ghys merged commit 123a8d3 into openhab:main Dec 13, 2023
6 checks passed
florian-h05 added a commit to florian-h05/openhab-webui that referenced this pull request Dec 13, 2023
Follow-up for openhab#2193.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
florian-h05 added a commit that referenced this pull request Dec 13, 2023
Follow-up for #2193.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@mherwege mherwege deleted the buttongrid branch December 15, 2023 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI rebuild trigger a new Jenkins job
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants