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

oh-input: Fix editing a number item with unit and accept Enter key #3050

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

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Feb 2, 2025

When oh-input is set to a Number item with unit, the f7-input in 'numeric' mode cannot set its numeric value to the item state because it contains the unit.

This PR:

  • Display the number's unit after the input field for numeric type
  • Make numeric input field right-aligned
  • Improve input-elements alignment
  • Accept Enter key to send the value, with or without a sendButton
  • Honor useDisplayState config
  • Add min and max config
  • If the stateDescription on the item specifies minimum, maximum, and step, they will be used as the default, but it can be overridden by the widget's config
  • Tested against Dimensionless Percent
  • Tested against patterns with multiple format specifiers, e.g. Test format %1$.4f and %1$.2f %unit%. The last format specifier will be used.
image

Copy link

relativeci bot commented Feb 2, 2025

#2778 Bundle Size — 10.99MiB (+0.03%).

57d5932(current) vs 95844d2 main#2743(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Regression 2 regressions
                 Current
#2778
     Baseline
#2743
Regression  Initial JS 1.91MiB(+0.16%) 1.9MiB
Regression  Initial CSS 577.29KiB(+0.01%) 577.21KiB
Change  Cache Invalidation 23.98% 17.51%
No change  Chunks 227 227
No change  Assets 250 250
No change  Modules 2952 2952
No change  Duplicate Modules 154 154
No change  Duplicate Code 1.8% 1.8%
No change  Packages 98 98
No change  Duplicate Packages 2 2
Bundle size by type  Change 7 changes Regression 7 regressions
                 Current
#2778
     Baseline
#2743
Regression  JS 9.2MiB (+100%) undefined
Regression  CSS 867.4KiB (+100%) undefined
Regression  Fonts 526.1KiB (+100%) undefined
Regression  Media 295.6KiB (+100%) undefined
Regression  IMG 140.74KiB (+100%) undefined
Regression  HTML 1.38KiB (+100%) undefined
Regression  Other 871B (+100%) undefined

Bundle analysis reportBranch jimtng:ohinput-numericProject dashboard


Generated by RelativeCIDocumentationReport issue

@mherwege
Copy link
Contributor

mherwege commented Feb 2, 2025

In the sitemap UI, we went the extra mile to split of the unit in that case and show it separately behind the input field. Maybe it is worth looking at that.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 2, 2025

indeed, more work is needed here, so we can also see the unit. Some layout improvements would be nice too.

@jimtng jimtng marked this pull request as draft February 2, 2025 11:45
@mherwege
Copy link
Contributor

mherwege commented Feb 2, 2025

Will this respect the number of decimal positions from the state description?

@jimtng
Copy link
Contributor Author

jimtng commented Feb 2, 2025

Will this respect the number of decimal positions from the state description?

Thanks! No it currently would not, but I'll work on that too.

Sample here with state desc %.2f %%

image

@jimtng jimtng changed the title oh-input: Fix editing a number item with unit oh-input: Fix editing a number item with unit and accept Enter key Feb 2, 2025
@jimtng
Copy link
Contributor Author

jimtng commented Feb 2, 2025

@mherwege I've updated the first post above with the full detail of the changes + latest screenshot

@jimtng jimtng marked this pull request as ready for review February 2, 2025 14:02
Display the number's unit after the input field for numeric type
Make numeric input field right-aligned
Improve input-elements alignment
Accept Enter key to send the value, with or without a sendButton
Honor useDisplayState config

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

mherwege commented Feb 2, 2025

Where does the unit come from? If it is the item unit, that is meant to be for internal use, not representation. That will mix things again, while we separated them by introducing the item unit. The unit used should be from the state description. E.g. your unit is default (W) but you want to show and change in kW.
With sitemap widgets, there is also the possibility to override the state description in the label for that one wiget. So it may even be necessary to introduce an extra widget parameter for the oh-input-widget.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 2, 2025

Where does the unit come from? If it is the item unit, that is meant to be for internal use, not representation. That will mix things again

Thanks! I'll make the adjustment.

With sitemap widgets, there is also the possibility to override the state description in the label for that one wiget. So it may even be necessary to introduce an extra widget parameter for the oh-input-widget.

Yes, but this needs to be done in core so it can perform unit conversion. The ui can't do that (or do we need to create a unit conversion REST endpoint?)

Say the item's unit and stateDescription is W and the widget specified unit is kW. How does the widget display a value of say 1000 W in its specified unit of kW?

@jimtng
Copy link
Contributor Author

jimtng commented Feb 3, 2025

OK, the next iteration will honor the unit specified in displayState, and if it contains %unit%, then fall back into the main item's unit.

image

image

@mherwege
Copy link
Contributor

mherwege commented Feb 3, 2025

Great. Sorry to go through iterations on this. My memory seems to come back in fragments. Can you also test with other locales that use , instead of . as the decimal separator? I believe that was the next challenge I ran into with basicUI.
And then also check with Dimensionless unit ONE and unit % for the different configuration options.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 3, 2025

@mherwege This is a bit more complicated than it may seem.

What do we do when the state description pattern is something like this:

I am part of stateDescription pattern %1$.2f W %1$.3f blah kW
image

Note it contains two patterns, one with two decimal digits, the second one with two decimal digits. I believe we should use the last one for editing. I know this is a crazy pattern, but it is possible

NOTE: it's already handled in my current code, but not yet pushed.

Great. Sorry to go through iterations on this. My memory seems to come back in fragments. Can you also test with other locales that use , instead of . as the decimal separator? I believe that was the next challenge I ran into with basicUI.
And then also check with Dimensionless unit ONE and unit % for the different configuration options.

Thank you for sharing your experiences! Saves me from finding all these bugs later on down the line.

I am still working through the above, and I'll incorporate your latest cases too.

@mherwege
Copy link
Contributor

mherwege commented Feb 3, 2025

Note it contains two patterns, one with two decimal digits, the second one with two decimal digits. I believe we should use the last one for editing. I know this is a crazy pattern, but it is possible

Yes, I am aware. I and also made an assumption there of only having on pattern. I also think there are places in core where it would fail when there is more than one.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 3, 2025

I also think there are places in core where it would fail when there is more than one.

indeed, I believe there are many places that would get tripped up with this, in webui, basicui, too.

Thankfully, I think I can rely on the fact that the unit is always the last token in the pattern, and that this isn't possible:
%1$.1f W %1$.4f kW (i.e. two different units in the same pattern)

Signed-off-by: Jimmy Tanagra <[email protected]>
@florian-h05 florian-h05 added bug Something isn't working main ui Main UI labels Feb 3, 2025
@jimtng
Copy link
Contributor Author

jimtng commented Feb 4, 2025

@mherwege

Finnish locale, amongst others, use a space as group (thousand) separator, whilst some others use comma / period for group separator / decimal and sometimes in reverse.

Additionally... f7-input doesn't want to deal with group separators, so they'd have to be stripped somehow.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 4, 2025

And then.... scientific notation %e

Extracting the number to be edited out of displayState is not as straight forward as it may seem. It is fraught with many challenges when dealing with complex patterns. For example, I can have a pattern like this:

999.%1$,.2f

The 999. is not a part of the value but it will be rendered in front of the value, e.g. 1.2 -> 999.1.20

Getting the raw number, then asking Core to convert it to the desired unit (and ignoring displayState completely) might be the cleanest, least hacky way I think. At the very least, we could still get the desired unit from displayState.

@florian-h05 wdyt?

@jimtng
Copy link
Contributor Author

jimtng commented Feb 4, 2025

As it is now, this PR will work fine provided that no funky patterns are used. i.e. no: numbers preceding the pattern not separated by space, no thousand separators, etc.

@mherwege
Copy link
Contributor

mherwege commented Feb 4, 2025

@jimtng

Finnish locale, amongst others, use a space as group (thousand) separator, whilst some others use comma / period for group separator / decimal and sometimes in reverse.

Yes, I know this is tricky. In BasicUI, a few regex expressions are used for this:

function parseNumber(value, unit) {

I believe these do a fair job on , vs . and scientific notation. But they are not perfect. Spaces for thousand separators will fail, although it might be possible to add that.

Additionally... f7-input doesn't want to deal with group separators, so they'd have to be stripped somehow.

I can't say what the problem is with that. Is that true for all browsers? When developing, I noticed there are some browser variations. Firefox seemed to be particularly challenging.

@mherwege
Copy link
Contributor

mherwege commented Feb 4, 2025

Getting the raw number, then asking Core to convert it to the desired unit (and ignoring displayState completely) might be the cleanest, least hacky way I think. At the very least, we could still get the desired unit from displayState.

Ideally, the numeric value converted to the display unit should just be part of the API response. But if I remember well the argument against it was it required adding extra fields in the SSE response. I don't think we want an extra REST call for each value to show in in input field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants