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

Add bind isChecked to onPress #502

Closed

Conversation

vladbelozertsev
Copy link

Hi.

I add bind to isChecked prop. Because when isChecked is defined as props from an app example: <BouncyCheckbox isChecked={someValue} />, and onPress action happened it don't see that isChecked is defined and set new checked (by toggling current checked value) regardless of isChecked prop. And we have two different states - lib state checked for example is false and app state checked for example is true. It means we have unchecked (false) checkbox on screen and checked (true) state app value.

Please check the changes, thanks!

@WrathChaos WrathChaos self-requested a review May 31, 2024 12:45
@WrathChaos WrathChaos self-assigned this May 31, 2024
@WrathChaos
Copy link
Owner

Hello @vladbelozertsev

Thank you so much for the contribution, let me review and test it :)

@vladbelozertsev
Copy link
Author

Hi. I add little changes that improve code quality.

@vladbelozertsev
Copy link
Author

vladbelozertsev commented Jun 24, 2024

Hi @WrathChaos. Is there any problem? In some cases we need uncheckable checkboxes or checkable after some validation. After disableBuiltInState been removed i cant make this. Could you please merge this pr or provide similar functional?

@WrathChaos
Copy link
Owner

Hey @vladbelozertsev

Extremely sorry, I could not spend time on open source nowadays but I am working on it finally :)

I checked the code and your explanation but I don't understand some things so please explain it to me.

if (isChecked === undefined) setChecked(!checked, onPress);
else if (onPress) onPress(isChecked);

Like this one, I am assuming you're controlling the check status with useState or something else on your side to put it as isChecked prop to change the checkbox state.


  const [checkboxState, setCheckboxState] = React.useState(false);
  
   <BouncyCheckbox
        size={50}
        textStyle={styles.textStyle}
        style={{marginTop: 16}}
        iconImageStyle={styles.iconImageStyle}
        fillColor={'#00C0EE'}
        unFillColor={'transparent'}
        ref={bouncyCheckboxRef}
        isChecked={checkboxState}
        text="Synthetic Checkbox"
        onPress={() => setCheckboxState(!checkboxState)}
      />
      <RNBounceable
        style={styles.syntheticButton}
        onPress={() => {
          bouncyCheckboxRef.current?.onCheckboxPress();
        }}>
        <Text style={{color: '#fff'}}>Synthetic Checkbox Press</Text>
      </RNBounceable>
  Like in the `manual example` you can control the whole checkbox without using the `onPress`'s newValue. It will not matter the bouncy checkbox's inner checked status because you're simply controlling your own state with `isChecked` and since you know the value of the state you do not need to know state coming from the bouncy checkbox anymore. 
  
  I believe this example is what you're looking for am I right? 

https://github.com/WrathChaos/react-native-bouncy-checkbox/tree/master/exampleManual

  You can check/run the whole example in here how to control bouncy checkbox from outside. 

@vladbelozertsev
Copy link
Author

vladbelozertsev commented Jul 16, 2024

Hi @WrathChaos.

Like in the manual example you can control the whole checkbox without using the onPress's newValue. It will not matter the bouncy checkbox's inner checked status because you're simply controlling your own state with isChecked and since you know the value of the state you do not need to know state coming from the bouncy checkbox anymore.

In this example we try to sync developer outer state with the inner lib state. But it doesn't work if after some validation inside onPress we don't toggle our outer state, and we have two different states - developer outer state for example true and lib inner state for example false. Because inner lib state don't have the same validation, and always toggle value.

Like this one, I am assuming you're controlling the check status with useState or something else on your side to put it as isChecked prop to change the checkbox state.

Yes, main idea is: if developer provide his own state by passing <BouncyCheckbox isChecked={developerStateValue} prop we sync the lib state with the developer state (its already done by useEffect with isChecked dependency).

But in current moment lib can break this logic by call setChecked func inside onPress/onLongPress actions. I.e. regardless of isChecked={developerStateValue} prop, lib can toggle own state that controlls the checkbox. It means developer cant control checkbox (by prop isChecked={developerStateValue}) and checkbox always can be toggled by user.

This code fix the issue:
if (isChecked === undefined) setChecked(!checked, onPress);
else if (onPress) onPress(isChecked);

if developer don't provide their own state i.e. isChecked === undefined, we toggle lib state (by setChecked(!checked, onPress)). Otherwise if onPress exists we call it with isChecked value (which is not undefined ie provide by developer from props) and don't toggle lib state. When isChecked={developerStateValue} prop have changed lib sync this value with their own checked state (by useEffect), i describe it above.

@vladbelozertsev
Copy link
Author

vladbelozertsev commented Jul 16, 2024

Imo for those who want use only lib state mb will be better to provide additional props value, something like isCheckedDefault. Who want to use their own state can set isChecked prop, otherwise we can set isCheckedDefault (initial value for the lib state) These two props values (isChecked/isCheckedDefault) are incompatible. I can write some additional code... Wait for response, thanks!

@WrathChaos
Copy link
Owner

Hello @vladbelozertsev
Thank you so much for the detailed explanation! I see your problem now, don't get me wrong I am just thinking we can provide a better solution than the checking if the isChecked is undefined because it will be hard to understand from other developers.

If possible can you give me a minimum example so I can work on it to reproduce the same issue and try to provide a bit better solution for everyone?

@vladbelozertsev
Copy link
Author

vladbelozertsev commented Jul 22, 2024

Hi @WrathChaos

If possible can you give me a minimum example so I can work on it to reproduce the same issue and try to provide a bit better solution for everyone?

Yes np. Its example from my app, that i currently working on. As you can see, here is the basic services, that user can't toggle (base tariff, documents fee) and additional services, that user can add / remove.

Screenshot_1721627500

@WrathChaos
Copy link
Owner

Hi @WrathChaos

If possible can you give me a minimum example so I can work on it to reproduce the same issue and try to provide a bit better solution for everyone?

Yes np. Its example from my app, that i currently working on. As you can see, here is the basic services, that user can't toggle (base tariff, documents fee) and additional services, that user can add / remove.

Screenshot_1721627500

Do you mind share the code with me? I only care the states, checkboxes and how you handle the checkboxes part. You can send me an email if you want it privately: [email protected]

@vladbelozertsev
Copy link
Author

vladbelozertsev commented Jul 22, 2024

This is a code from the example above. I use my own BouncyCheckbox component (import BouncyCheckbox from 'src/components/checkbox';) with the changes that i provide in current pr. I.e. in this code BouncyCheckbox component can't be toggled by user.

import BouncyCheckbox from 'src/components/checkbox';
import React, { FC } from 'react';
import { AddBookingForm } from 'src/api/bookings';
import { MyText } from 'src/components/my-text';
import { RouteTariffAPI } from 'src/types/route-api';
import { ScaledSheet, ms } from 'react-native-size-matters';

type Props = {
  tarifs?: RouteTariffAPI[];
  field: Field<AddBookingForm, 'CheckedTariffs'>;
  fieldState: FieldState;
};

export const Tariffs: FC<Props> = props => {
  const { tarifs, field } = props;

  return tarifs?.map(tr => {
    const arr = field?.value || [];
    const isChecked = arr.includes(tr.ID);
    const isRequired = tr.Required?.toString() === '1' || tr.Group === '1';
    const addChecked = () => field.onChange([...arr, tr.ID]);
    const delChecked = () => field.onChange(arr.filter(id => id !== tr.ID));

    return (
      <BouncyCheckbox
        fillColor={CLR_APP}
        iconImageStyle={{ width: ms(12), height: ms(12) }}
        iconStyle={styles.checkboxContainer}
        innerIconStyle={styles.innerIcon}
        isChecked={isRequired || isChecked}
        key={tr.ID}
        onPress={isRequired ? undefined : isChecked ? delChecked : addChecked}
        size={ms(34)}
        style={styles.checkbox}
        textComponent={<MyText $v={tr.Name + ' ' + +tr.Value + '$'} $fs="16" />}
      />
    );
  });
};

const styles = ScaledSheet.create({
  checkbox: {
    marginBottom: '25@ms',
  },
  checkboxContainer: {
    borderColor: '#FFF',
    marginRight: '5@ms',
    borderRadius: 5,
  },
  innerIcon: {
    borderRadius: 5,
    borderWidth: 3,
  },
});

@WrathChaos
Copy link
Owner

@vladbelozertsev
Thank you so much for the extra code example. It's been crazy busy week, I will take care of it tomorrow 🙏 Sorry for postponing this PR so much :(

@vladbelozertsev
Copy link
Author

vladbelozertsev commented Aug 11, 2024

hi @WrathChaos.
I think its will be a good solution to add extra prop useBuiltInState, it can get boolean or object value with initial state value. I can add this simple logic to current pr.

And we will have 2 cases:

  1. developers can use their own state outside component and control checkbox value by isChecked prop
  2. use lib built in state by set useBuiltInState prop

Examples:
use built in state: <BouncyCheckbox useBuiltInState />
use built in state with initial value: <BouncyCheckbox useBuiltInState={{initialValue: false}} />

I think its right solution, because when we pass isChecked prop it makes illusion that checkbox state value always been controlled by this (isChecked) value.

@DTX-Elliot
Copy link

This fix would be greatly appreciated!

@WrathChaos
Copy link
Owner

WrathChaos commented Aug 19, 2024

Hello @vladbelozertsev and @DTX-Elliot
First of all I apologize for extremely late action.

I released 4.1.1 version with the useBuiltInState prop and updated the README with how to use it.

Simply we just need to useBuiltInState={false} and set your own state to isChecked prop and that's all. Please, please give me feedback if it is working or not.

Thank you so much for the patience!

@WrathChaos
Copy link
Owner

WrathChaos commented Aug 21, 2024

New version is available to break the breaking change 🤭 Please move to 4.1.2

@vladbelozertsev

@WrathChaos WrathChaos closed this Aug 21, 2024
@vladbelozertsev
Copy link
Author

Sorry for the late answer. Its worked for me, thanks!

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