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.

Sign up for GitHub

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

Jump to bottom

Add triple state switch widget #1160

Merged
merged 6 commits into from
Jan 25, 2024
Merged

Add triple state switch widget #1160

merged 6 commits into from
Jan 25, 2024

Conversation

vinothvino42
Copy link
Contributor

@vinothvino42 vinothvino42 commented Jan 22, 2024 •
edited
Loading

Ticket: #1154

value = off | mixed | on

Doc - https://github.com/EnsembleUI/ensemble_docs/pull/123

Sample EDL

View:
  header:
    title: Triple Switch
  body:
    Column:
      styles: { gap: 24, padding: 24, scrollable: true }
      children:
        - TripleSwitch:
            leadingText: "Entire Place "
            value: mixed
            enabled: true
            styles:
              {
                activeThumbColor: white,
                inactiveThumbColor: grey,
                activeColor: 0xFFD6D6CE,
                inactiveColor: 0xFF425C59,
                mixedColor: 0xFF858174,
              }
            onChange: |
              //@code
              console.log("Switch Value: "+this.value);
Simulator.Screen.Recording.-.iPhone.15.-.2024-01-22.at.17.13.41.mp4

@vinothvino42 vinothvino42 added the enhancement New feature or request label Jan 22, 2024
@vinothvino42 vinothvino42 self-assigned this Jan 22, 2024
@vinothvino42 vinothvino42 mentioned this pull request Jan 22, 2024
Closed
snehmehta
snehmehta approved these changes Jan 22, 2024
View reviewed changes
assets/schema/ensemble_schema.json
@@ -1849,6 +1849,35 @@
]
},
"Switch-payload": {
"type": "object",
Copy link
Contributor

Choose a reason for hiding this comment

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

hey vinoth, the schema needs to add in ensemble-web-studio ts files, check the studio repo readme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I'll update in the studio once this PR is merged

lib/widget/triple_state_switch.dart Outdated
void onToggle(SwitchState newValue) {
widget.onToggle(newValue);
widget._controller.value = newValue;
if (widget._controller.onChange != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a dispatch event here, so if any listener is bind to switch state change will update as well.
Something like this

ensemble/lib/layout/tab_bar.dart

Lines 160 to 161 in afa3e8d

scopeManager.dispatch(
ModelChangeEvent(

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be automatic right @snehmehta ? We shouldn't have to dispatch anythign for binding to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, my bad. It works. For some reason i thought the case of tabbar and footer not updating on tab change because of event but I was wrong it update I check my theory.

lib/widget/triple_state_switch.dart Outdated
widget: FormField<bool>(
key: validatorKey,
validator: (value) {
if (widget._controller.required) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do test this validation case, from the look of it won't work as expected

vusters
vusters reviewed Jan 22, 2024
View reviewed changes
lib/widget/triple_state_switch.dart Outdated
import 'package:ensemble_ts_interpreter/invokables/invokable.dart';
import 'package:flutter/material.dart';

class EnsembleTripleStateSwitch extends StatefulWidget
Copy link
Contributor

Choose a reason for hiding this comment

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

see Avatar for new widget implementation. This is the legacy way of building a widget.

Copy link
Contributor Author

@vinothvino42 vinothvino42 Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, the Switch widget is using the FormFieldController which is not migrated to the new way (extending the EnsembleWidgetController instead of WidgetController). I tried changing the FormFieldController so I am supposed to migrate all the widget which uses the FormFieldController to new way of building widgets (EnsembleWidget).

So as of now I done in the old way only

lib/widget/triple_state_switch.dart Outdated
void onToggle(SwitchState newValue) {
widget.onToggle(newValue);
widget._controller.value = newValue;
if (widget._controller.onChange != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be automatic right @snehmehta ? We shouldn't have to dispatch anythign for binding to work.

lib/widget/triple_state_switch.dart Outdated
}

@override
Map<String, Function> setters() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why copy and pasted all these code? 99% of it identical to the regular switch. You should at least use a base class instead of replicating all these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored

vusters
vusters reviewed Jan 22, 2024
View reviewed changes
lib/widget/triple_state_switch.dart Outdated
}
}

enum SwitchState {
Copy link
Contributor

Choose a reason for hiding this comment

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

use on, off, and mixed as these are more user friendly. Start and End doesn't mean much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it

vusters
vusters reviewed Jan 22, 2024
View reviewed changes
lib/widget/triple_state_switch.dart Outdated
Map<String, Function> setters() {
return {
'value': (value) => _controller.value =
SwitchState.values.from(Utils.getString(value, fallback: 'start')) ??
Copy link
Contributor

Choose a reason for hiding this comment

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

You have two "start" values here plus the default "start" - that is 3 different places to change if you decide to change the default state. Potential cause for bug unnecessary.
Use this:
_controller.value = SwitchState.values.from(value) ?? _controller.value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@vinothvino42 vinothvino42 requested a review from vusters January 24, 2024 04:50
vusters
vusters reviewed Jan 24, 2024
View reviewed changes
lib/widget/switch.dart Outdated
'leadingText': (text) =>
_controller.leadingText = Utils.optionalString(text),
'trailingText': (text) =>
_controller.trailingText = Utils.optionalString(text),
'activeColor': (color) => _controller.activeColor = Utils.getColor(color),
'inactiveColor': (color) =>
_controller.inactiveColor = Utils.getColor(color),
'intermediateColor': (color) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

mixedColor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it

lib/widget/switch.dart Outdated
@@ -50,28 +61,30 @@ class EnsembleSwitch extends StatefulWidget
};
}

void onToggle(bool newValue) {
setProperty('value', newValue);
void setSwitchValue(dynamic value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Switch and TripleSwitch's value should be succinct. The former takes in true/false, the latter 3 states. There should be no guessing like this, so please refactor so their inputs are separate. The schema should reflect tis too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added onToggle in those widgets with their state as input parameter

lib/widget/switch.dart Outdated

framework.EnsembleAction? onChange;
void onToggle(dynamic newValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing here. Each widget should have its own onToggle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it

lib/widget/switch.dart Outdated
@@ -102,12 +115,13 @@ class SwitchState extends FormFieldWidgetState<EnsembleSwitch> {

// wraps around FormField to get all the form effects
return InputWrapper(
type: EnsembleSwitch.type,
// type: EnsembleTripleStateSwitch.type,
type: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

put the type in. This is to help with exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@vinothvino42
Copy link
Contributor Author

vinothvino42 commented Jan 25, 2024 •
edited
Loading

@vusters Can you review it? Just updated it, also checked the validation and it's working fine (It fixes this issue also #1155)

  • Moved the onToggle to Switch and TripleSwitch with their input as parameter and updated the logic in the SwitchBase class for value-changing
  • Changed the widget name from TripleStateSwitch to TripleSwitch
  • Added the type to the InputWrapper widget
  • Changed the property from intermediateColor to mixedColor

@vinothvino42 vinothvino42 requested a review from vusters January 25, 2024 06:28
@vinothvino42 vinothvino42 merged commit ff63c3d into main Jan 25, 2024
@kmahmood74 kmahmood74 mentioned this pull request Feb 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vusters vusters vusters approved these changes

@snehmehta snehmehta Awaiting requested review from snehmehta

Assignees

@vinothvino42 vinothvino42

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vinothvino42 @vusters @snehmehta