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

[BasicUI] Align and optimize available space for switch with mappings #2388

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Feb 22, 2024

  • Should only affect switch with mappings, not player control
  • Utilise the wasted space instead of wrapping the buttons into multiple rows, if possible.
  • Right align the buttons. This makes it look much neater.
  • Instead of limiting the width of the buttons, reserve a minimum width for the label
  • However if the label is shorter than 6 characters (including blank labels), reduce the label's minimum width to just what's actually taken up by the shorter label. This gives more space for the buttons with shorter labels.
  • When buttons wrap to multiple rows, make sure that each row contains almost the same number of buttons, instead of having the first row filling up the horizontal space, move the buttons down. By doing this, there is more space for the label to occupy.
  • Squeeze extra space for more buttons in "condensed layout" by reducing padding, inter-button gaps, min-width, etc.
  • The reduced padding in condensed layout also affects buttons in buttongrid.

Before:
image

After:

image

@jimtng jimtng requested a review from a team as a code owner February 22, 2024 11:38
@lolodomo
Copy link
Contributor

The idea was to keep a minimum of place for label and value.

@lolodomo
Copy link
Contributor

Your commit of vscode settings is probably unexpected ?

@jimtng
Copy link
Contributor Author

jimtng commented Feb 22, 2024

The idea was to keep a minimum of place for label and value.

I haven't tested this with various widths of labels.

Your commit of vscode settings is probably unexpected ?

oops, removed now.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 22, 2024

The idea was to keep a minimum of place for label and value.

Reserving space for the label and value has merits too. I see that simply removing the width here is not enough.

Before:
image

After (bad for the label):
image

I'll play with it a bit more, and I'm open to ideas

@lolodomo lolodomo added the basic ui Basic UI label Feb 23, 2024
@jimtng jimtng marked this pull request as draft February 23, 2024 17:07
@lolodomo
Copy link
Contributor

lolodomo commented Mar 6, 2024

Using 4.2 M1, I can see that I have to increase the zone for buttons to not have controls for a Player item cut on 2 lines on a phone.

And I also see that the buttons in the new settings page are not rendered perfectly on a phone when cut on 2 lines.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 7, 2024

I haven't had a chance to look into this yet, and I'm generally clueless with css. If you have a fix, feel free post it and close this PR.

Ideally, the buttons can take up as much horizontal space as possible, especially when there are no labels or the label is short. This way user can set a blank label to maximise the space for the buttons. But when there's a label, then reserve some space for the label.

@lolodomo
Copy link
Contributor

lolodomo commented Mar 9, 2024

Even with your change, I have strange result like this one:

image

If I increase just a little the window, it is then OK:

image

Maybe the problem occurs when the label is truncated and "..." is injected ?

@lolodomo
Copy link
Contributor

lolodomo commented Mar 9, 2024

I would like to find the proper CSS properties to at least have player controls always on one line when there is enough place for that.

@lolodomo
Copy link
Contributor

lolodomo commented Mar 9, 2024

If I reduce the length of my label, this is then fine:

image

The problem seems to be when the label is truncated.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 9, 2024

The solution would probably include changing the css of the labels and/or the container.
Could we perhaps turn it into two lines (dynamically) when there's not enough space? So the label on top, then controls below it, but if there's enough space, keep them both in one line.

@jimtng
Copy link
Contributor Author

jimtng commented Jul 29, 2024

Original post updated with the latest result.

@jimtng jimtng changed the title Basic UI: remove max-width for buttons Basic UI: align and optimize available space for switch with mappings Jul 29, 2024
@jimtng jimtng marked this pull request as ready for review July 29, 2024 06:18
@jimtng
Copy link
Contributor Author

jimtng commented Jul 29, 2024

@lolodomo this still reserves some space for the label / value, but also tries to make the buttons take up as much space as possible, but if it must wrap into multiple lines, maximise the space for the label again. See the original post with updated screenshots

@jimtng
Copy link
Contributor Author

jimtng commented Jul 29, 2024

For anyone interested in testing, you can put this jar file in your addons folder, then disable the built in org.openhab.ui.basic bundle, and restart/enable this bundle.

Note: rename the file and remove the .txt extension.

org.openhab.ui.basic-4.3.0-SNAPSHOT.jar.txt

@GeVaSta
Copy link

GeVaSta commented Jul 29, 2024

New JAR is worse I think. Even wider screen does not put the buttons on one line.
Clipboard_07-29-2024_02

@jimtng
Copy link
Contributor Author

jimtng commented Jul 29, 2024

New JAR is worse I think. Even wider screen does not put the buttons on one line.

Make sure you do a force refresh (Cmd+Refresh on mac, shift+reload or something - depending on your browser).

@GeVaSta
Copy link

GeVaSta commented Jul 29, 2024

I did a refresh (Shift+F5 on Windows).
Looks like it could be something with the number of colums setting of the browser. I have mine on one (1) column.

@jimtng
Copy link
Contributor Author

jimtng commented Jul 29, 2024

Works fine for me, both 1 column and 2 columns.

image image image

@GeVaSta
Copy link

GeVaSta commented Jul 29, 2024

Clipboard_07-29-2024_03
Clipboard_07-29-2024_06
After playing with the number of colums it changed a bit but still plenty of room to add button to the right instead on two lines.

@jimtng
Copy link
Contributor Author

jimtng commented Jul 29, 2024

I suspect it hasn't fuly refreshed. What's your O/S, browser version? I'll try it on BrowserStack

@jimtng
Copy link
Contributor Author

jimtng commented Jul 29, 2024

This is on BrowserStack running on Windows 11, Edge 125:

image

@GeVaSta
Copy link

GeVaSta commented Jul 29, 2024

The difference is, I think, I use a much smaller window. Thats for me the whole point. I like the window as small as possible to put it right side of my screen to keep it visible.

@GeVaSta
Copy link

GeVaSta commented Jul 29, 2024

Can you show a screenshot of the smallest browser window where one button moved to a second line? And the smallest on one line please?

@jimtng
Copy link
Contributor Author

jimtng commented Jul 29, 2024

I see, so when the window is narrowed, it does wrap. Thanks! I'll make some adjustments to fix that.

image

Signed-off-by: Jimmy Tanagra <[email protected]>
@lolodomo
Copy link
Contributor

lolodomo commented Sep 14, 2024

Finally, on my desktop with Firefox and Chrome in full screen and same settings, I see the label for the element at right truncated in Firefox while not truncated in Chrome. Any idea why ? It is in not condensed layout.

image

image

Apparently (using dev tools), the size for the widget is a little bigger in case of Chrome: 507.86 vs 506.667 in Firefox.
When I look at global page, width is 1603.62 in Chrome vs 1600 in Firefox.
So it explains the result.
Nothing to change or fix I assume.

@lolodomo
Copy link
Contributor

lolodomo commented Sep 14, 2024

I take a look on the possible impact of your changes regarding mdl-form__row for a Buttongrid and a Chart.
No rendering change noticed in non condensed layout.
In condensed layout, I can see that the grid/chart has been moved to the left, that means the border at left is now smaller than at right. Before your change, it was not yet perfect but the reverse, bigger border at right than at left. In both cases, one of the border is too big, compared to non condensed layout. With your change, this is the border at right that is now too big.
In non condensed layout, the centering looks very good.
@jimtng : Even if this is not dramatic (and not new), would you like to try to fix that ? If not, could be the object of a next PR.

With your change (condensed layout):
image

Before your change (condensed layout):
image

In non condensed layout (with out without your change):
image

Note that the issue is global to any widget, not specific to Buttongrid or Chart.

@lolodomo
Copy link
Contributor

lolodomo commented Sep 14, 2024

My tests with my usual sitemap are finished and the only bug that remains is the re-introduction of the bug with the Player item (display of button,s forced on one line) that can lead to buttons moved too much to the right and overriding the widget at right. I will provide a sitemap to reproduce it but a simple Payer item should be sufficient to reproduce it when in non condensed layout. Setting bigger font option shows even more the problem.

@lolodomo
Copy link
Contributor

lolodomo commented Sep 14, 2024

The additional tests I would like to execute are when the value is displayed at left of the buttons and when icons are used inside buttons (check that button width is then still as expected).

@jimtng
Copy link
Contributor Author

jimtng commented Sep 14, 2024

In condensed layout, I can see that the grid/chart has been moved to the left, that means the border at left is now smaller than at right. Before your change, it was not yet perfect but the reverse, bigger border at right than at left. In both cases, one of the border is too big, compared to non condensed layout. With your change, this is the border at right that is now too big.
In non condensed layout, the centering looks very good.
@jimtng : Even if this is not dramatic (and not new), would you like to try to fix that ? If not, could be the object of a next PR.

Could you please provide a sitemap for me to test?

My tests with my usual sitemap are finished and the only bug that remains is the re-introduction of the bug with the Player item (display of button,s forced on one line) that can lead to buttons moved too much to the right and overriding the widget at right. I will provide a sitemap to reproduce it but a simple Payer item should be sufficient to reproduce it when in non condensed layout. Setting bigger font option shows even more the problem.

I also need a sitemap to show this issue.

@lolodomo
Copy link
Contributor

lolodomo commented Sep 14, 2024

My tests with my usual sitemap are finished and the only bug that remains is the re-introduction of the bug with the Player item (display of button,s forced on one line) that can lead to buttons moved too much to the right and overriding the widget at right.

I found what is your change that re-introduced the bug, it is a direct consequence to set min-width to 5em for the label. We are in the case that this minimum is too much and there is then no enough place to show the 4 player buttons.
If I reduce this min size to 1em for example, then it works well.

It is exactly what I said in my comment about this adding of a min width, we have to be sure that it is without any wrong consequence on any widget. It has one with the Player item.

Can you tell me if there is a real need to add such minimum width for the label ?

@lolodomo
Copy link
Contributor

lolodomo commented Sep 14, 2024

Can you tell me if there is a real need to add such minimum width for the label ?

In case it is usefull for multiline buttons, why not rather setting it only in JavaScript code and only for this widget when we are in multilines mode ?
It would have the big advantage that we are certain that this change will not impact other widgets.
WDYT ?

@jimtng
Copy link
Contributor Author

jimtng commented Sep 14, 2024

min-width for label is absolutely needed for the multiline buttons, because we removed the max-width on the buttons.

Would reducing the min-width work for the player? I would prefer that than setting it in javascript (or css) only for multiline buttons.

I don't care what it's reduced to, as long as it's set to something that's not too small that would cause the labels to be unreadable.

@lolodomo
Copy link
Contributor

min-width for label is absolutely needed for the multiline buttons, because we removed the max-width on the buttons.

Ok

Would reducing the min-width work for the player? I would prefer that than setting it in javascript (or css) only for multiline buttons.

Not sure to understand why you prefer this option.

I don't care what it's reduced to, as long as it's set to something that's not too small that would cause the labels to be unreadable.

I should then experiment what could be a reasonable value.

@jimtng
Copy link
Contributor Author

jimtng commented Sep 14, 2024

@lolodomo in the latest change, I only set the min-width for multiline buttons. Could you please test it against the player item?

@lolodomo
Copy link
Contributor

@lolodomo in the latest change, I only set the min-width for multiline buttons. Could you please test it against the player item?

Very good option. I will tell you if it works but I am already sure it will.

@lolodomo
Copy link
Contributor

[INFO] [15:50:42] Starting 'eslint'...
[INFO] [15:50:42] Starting 'snippetsAll'...
[INFO] Error in plugin "sass"
[INFO] Message:
[INFO]     web-src\_layout.scss
[INFO] Error: expected ':' after $--label-multiline in assignment statement
[INFO]         on line 235 of web-src/_layout.scss
[INFO]         from line 9 of web-src/smarthome.scss
[INFO] >>       $__label-multiline {
[INFO]
[INFO]    -^
[INFO]

@lolodomo
Copy link
Contributor

Typo at line 235: you used $ intsead of &

@jimtng
Copy link
Contributor Author

jimtng commented Sep 14, 2024

Typo at line 235: you used $ intsead of &

Thanks! I was scratching my head!

@lolodomo
Copy link
Contributor

Thanks! I was scratching my head!

What is strange is that the build was not failing despite this error. This is something we have to fix (in another PR).

@lolodomo
Copy link
Contributor

The bug is fixed, the rendering of the player item is fine on my phone either in portrait or paysage (2 columns) modes whatever the values of "condensed layout" and "bigger font size" parameters.

To be precise, it is still possible to reproduce the bug but it is not the consequence of any of your changes, it was already possible before. On a desktop screen with 3 columns, if you reduce the window width, at a certain time there is no more place to display the label at all and the buttons start to shift to the right to be fully rendered. In fact, the solution would be to use multiline buttons once there is no more place for the label. But this problem will be encountered in very rare cases so I propose to ignore it at this time. Another PR could be added later to switch from mono-line to multi-line in the JavaScript when the label size is 0.

@lolodomo
Copy link
Contributor

lolodomo commented Sep 15, 2024

When using icons in buttons, I have a cut in two lines when it could be displayed on one line (was before your change). Buttons with text on the first widget at left are on one line while the buttons take more place than with icons.

image

Before:

image

Sitemap:

frame {
Switch item=TestCommand mappings=[0=NULL,1=UNDEF,2="Set 1",3="Set 2"]
Switch item=TestCommand mappings=[0=NULL=f7:house,1=UNDEF="material:favorite",2="Set 1"=if:lucide:activity-square,3="Set 2"=chart]
Switch item=TestCommand mappings=[0=NULL=f7:house,1=UNDEF,2="Set 1"=if:lucide:activity-square,3="Set 2"]
}

This is in non condensed layout with standard font size.

@lolodomo
Copy link
Contributor

lolodomo commented Sep 15, 2024

Another annoying case: look at the widget on last line in the middle. Its size is bigger than expected at left and right. On my phone, I can see that it can also happen even without the display of the value, that is for the widget just before (last line at left). This is in non condensed layout with bigger font size.

image

Before:

image

Sitemap:

    Frame {
        Switch item=DemoString mappings=[ VALUE1="Val 1", VALUE2="Value 2"]
        Switch item=DemoString label="Demo String [%s]" mappings=[ VALUE1="Val 1", VALUE2="Value 2"]
        Switch item=DemoString mappings=[ VALUE1="Val 1", VALUE2="Value 2", VALUE3="Val 3"]
        Switch item=DemoString label="Demo String [%s]" mappings=[ VALUE1="Val 1", VALUE2="Value 2", VALUE3="Val 3"]
        Switch item=DemoString mappings=[ VALUE1="Val 1", VALUE2="Value 2", VALUE3="Val 3", VALUE4="Value 4"]
        Switch item=DemoString label="Demo String [%s]" mappings=[ VALUE1="Val 1", VALUE2="Value 2", VALUE3="Val 3", VALUE4="Value 4"]
        Switch item=DemoString mappings=[ VALUE1="Val 1", VALUE2="Value 2", VALUE3="Val 3", VALUE4="Value 4", VALUE5="Val 5", VALUE6="Long value 6", VALUE7="An ultra mega long value 7"]
        Switch item=DemoString label="Demo String [%s]" mappings=[ VALUE1="Val 1", VALUE2="Value 2", VALUE3="Val 3", VALUE4="Value 4", VALUE5="Val 5", VALUE6="Long value 6", VALUE7="An ultra mega long value 7"]
        Default item=DemoString label="Demo String [%s]"
    }

@jimtng
Copy link
Contributor Author

jimtng commented Sep 15, 2024

When using icons in buttons, I have a cut in two lines when it could be displayed on one line

I'm investigating this

Another annoying case: look at the widget on last line in the middle.

It seems that the "after" has a min-width that's wider than the "before". The advantage is we can read more of the label, at the expense of a narrower space for the buttons.

If you prefer to sacrifice the space for the label, then we can simply reduce the min-width.

Its size is bigger than expected at left and right

Can you please clarify this?

@lolodomo
Copy link
Contributor

lolodomo commented Sep 15, 2024

Can you please clarify this?

Look at the alignment of icons at left and the buttons at right. They are no more aligned.
On my phone, it even leads to a truncated icon and truncated buttons at right.

@jimtng
Copy link
Contributor Author

jimtng commented Sep 15, 2024

When using icons in buttons, I have a cut in two lines when it could be displayed on one line

I'm investigating this

OK this one is solved and will be in the next commit. When using (some types of) icons, the icons weren't loaded yet when the minimizeWidth() was initially executed, resulting in an incorrect calculation.

Another annoying case: look at the widget on last line in the middle. Its size is bigger than expected at left and right.

This is a result of the label's min-width. If you compare to "before" notice the label's size was smaller to compensate.

However, try make your button even wider, e.g.

        Switch item=DemoString label="Demo String [%s]" mappings=[ VALUE1="Val 1", VALUE2="Value 2", VALUE3="Val 3", VALUE4="Value 4", VALUE5="Val 5", VALUE6="Long value 6", VALUE7="An ultra mega long value 7 supercalifragilisticexpialidocious"]

Even the "before" will also suffer the same problem, although the "after" version is worse because buttons have no max-width to limit it.

I'm not sure of the best way to solve this because either way, something needs to either be truncated, or pushed around. Perhaps truncation is better because it won't affect the neighbouring controls.

@lolodomo
Copy link
Contributor

Perhaps truncation is better because it won't affect the neighbouring controls.

Yes I believe.

@jimtng
Copy link
Contributor Author

jimtng commented Sep 15, 2024

@lolodomo please test the latest changes

@lolodomo
Copy link
Contributor

lolodomo commented Oct 6, 2024

@jimtng : the last version is worst I think because now labels are truncated even when there is place to not truncate them.

image

Maybe we should switch to your previous version which was not so bad even if not perfect. WDYT ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
basic ui Basic UI enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants