SAP / fundamental-ngx

Fundamental Library for Angular is SAP Design System Angular component library
https://sap.github.io/fundamental-ngx
Apache License 2.0
271 stars 130 forks source link

Segmented button value conflict #10119

Open ZsDeak-SAP opened 1 year ago

ZsDeak-SAP commented 1 year ago

Is this a bug, enhancement, or feature request?

bug

Briefly describe your proposal.

Which versions of Angular and Fundamental Library for Angular are affected? (If this is a feature request, use current version.)

tried with Angular 15 and fundamental 40.3, but the code is the same on master right now

If this is a bug, please provide steps for reproducing it.

Use the component in toggle mode, starting with empty state. Assign the clicked button's value to the current value in a handler.

Expected: the value defined by the parent component through writeValue is presented by the component

Actual: the inner handler overrides the button's toggled state. what i saw in the code was a call first from writeValue that selected the button, but then the segmented-button component's own event handler deselected it. image

Please provide relevant source code if applicable.

<fd-segmented-button [toggle]="true" [ngModel]="currentValue">
  <button
    *ngFor="let value of values"
    fd-button
    [label]="value"
    [attr.value]="value"
    (click)="handleValueChange(value)"
  ></button>
</fd-segmented-button>

<br />
<small>value: {{ currentValue }}</small>
import { Component } from '@angular/core';

@Component({
  selector: 'fd-segmented-button-complex-example',
  templateUrl: './segmented-button-complex-example.component.html',
})
export class SegmentedButtonComplexExampleComponent {
  values: string[] = ['first', 'second', 'third'];
  currentValue = [];

  handleValueChange(value: string): void {
    this.currentValue = [value];
  }
}
khotcholava commented 2 months ago

@ZsDeak-SAP Hello, I did not understand well what is the problem, Can you please attack stackblitz example?

ZsDeak-SAP commented 2 months ago

@khotcholava it's the fd-segmented-button-complex-example from the fundamental examples, only with toggle mode and array value, but here's the same copy pasted to stackblitz😄 :

https://stackblitz.com/edit/tlhuhf?file=src%2Fapp%2Fsegmented-button-complex-example.component.ts

When you use toggle mode, value accepts an array. In this example I oversimplified handleValueChange, but it's still valid, it sets an array of values. As you can see the selected value is written under the buttons, but there's no highlight on the selected button.

khotcholava commented 2 months ago

@ZsDeak-SAP Thank you for the information. Could you please take a look at this StackBlitz example? I made some adjustments to your example to correctly handle the currentValue. Previously, currentValue was being set for only one item at a time, but now it works as expected. Let me know if you still encounter any issues.

ZsDeak-SAP commented 2 months ago

@khotcholava my bad, I incorrectly copied my own code as shown above. handleValueChange should be on the button. pls check now. https://stackblitz.com/edit/tlhuhf

ZsDeak-SAP commented 2 months ago

it's a matter of when _handleTriggerOnButton is called, before or after writeValue. normally it's called before, so _handleTriggerOnButton sets the toggled to true and then comes writeValue which also sets it to true. Now if the handleValueChange is on the child buttons, then event bubbling will reach it sooner than fd-segmented-button, so then writeValue will set toggled to true, and then handleValueChange will toggle it back to false.

As I see it right now this is pretty much a catch 22, there are cases when only one of these 2 methods is used, there are cases when only the other and there's this one case when both and the order is not right.

I think we should add a similar note as Angular Material has (I guess because of the same issue) https://v5.material.angular.io/components/button-toggle/overview#exclusive-selection-vs-multiple-selection