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

UI: modal-line element INPUT with optional unit #2489

Merged
merged 4 commits into from
Mar 8, 2024

Conversation

JosefRick
Copy link
Contributor

Implements a new modal-line element INPUT_UNIT. It's the similar to INPUT with the option to change the unit with the following code line.
eg: [control]="{type: 'INPUT_UNIT', properties: {unit: 'mA'}}">

Implement INPUT_UNIT for INPUT with changable Unit
Added INPUT_UNIT as new control element
Copy link

Code Coverage

@JosefRick JosefRick changed the title New modal-line element INPUT_UNIT UI: new modal-line element INPUT_UNIT Jan 17, 2024
Copy link
Contributor

@lukasrgr lukasrgr left a comment

Choose a reason for hiding this comment

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

this is a design error, i would add a optional unit to the control object and use "W" as fallback

Copy link

Code Coverage

1 similar comment
Copy link

Code Coverage

@JosefRick
Copy link
Contributor Author

Yes, that is a better option. I have changed it now. Please take a look at it.

@JosefRick JosefRick changed the title UI: new modal-line element INPUT_UNIT UI: modal-line element INPUT with optional unit Jan 17, 2024
Copy link
Contributor

@lukasrgr lukasrgr left a comment

Choose a reason for hiding this comment

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

i think there is still a cleaner solution for this. Create a Enum e.g. InputUnits, that holds in this case only WATT = 'W'. Use this then as the type for the unit. Define the defaultValue in typescript then

@JosefRick
Copy link
Contributor Author

I'm not sure if this is really a clean solution, because for the control RANGE there is also the same way to define the unit if i understand it correct. So if we do it with Enum now, there are different possibilities for each control and so I think it is more difficult to understand which definition is the right one for each control. An alternative idea is, we make a new unit.ts in the shared folder so that every unit definition on the genericComponents can use it and it is defined for all at one point.

@lukasrgr
Copy link
Contributor

lukasrgr commented Feb 2, 2024

@JosefRick so you would define a fallback type "W" in this, do i understand correctly?

@JosefRick
Copy link
Contributor Author

@lukasrgr Yes, with this code I have defined a fallback 'W' if no properties are specified in the control type.
For example [control]="{type: 'INPUT', properties: {unit: 'mA'}}" in the modal.html will display the unit mA, and if no properties are specified, like this [control]="{type: 'INPUT'}" the code will use W as the unit.

@lukasrgr lukasrgr requested a review from sfeilmeier March 6, 2024 13:24
Copy link
Contributor

@sfeilmeier sfeilmeier left a comment

Choose a reason for hiding this comment

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

Ok, thanks!

@lukasrgr: What's the reason for keeping unit "W" as default? I actually think there should be no default unit here. If none is given, none should be displayed. Of course this requires changes in quite a few files to explicitely set the unit there.

@sfeilmeier sfeilmeier merged commit 9031dbb into OpenEMS:develop Mar 8, 2024
2 checks passed
fanass-dev pushed a commit to fanass-dev/openems that referenced this pull request May 6, 2024
Implements a new modal-line element INPUT_UNIT. It's the similar to INPUT with the option to change the unit with the following code line.
eg: [control]="{type: 'INPUT_UNIT', properties: {unit: 'mA'}}">
@JosefRick JosefRick deleted the Modal-Line-INPUT_UNIT branch September 27, 2024 12:17
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