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

Switched the CheckboxInput component to use Checkbox.Item to stay mor… #56

Merged
merged 4 commits into from
Apr 18, 2024

Conversation

himrocks33
Copy link

Switched the CheckboxInput component to use Checkbox.Item to stay more consistent with RN paper and allow for the mode parameter.

This will use Text from RN Paper for the label as well keeping the theme consistent.

…e consistent with RN paper and allow for the mode parameter.
Copy link
Contributor

@Thodor12 Thodor12 left a comment

Choose a reason for hiding this comment

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

React native paper uses checkboxes after the line of text, that doesn't look good in the select.
I would not accept the PR in the current state without providing an alternative to still have the text default to being right aligned.

@himrocks33
Copy link
Author

@Thodor12 Please note in this PR I have set the 'position' property to 'leading'. This puts the checkbox on the left side of the text. The implementation is the same as before. It just stays more consistent with React Native Paper's styling. It also adds the mode property. Thanks for the review.

… for RNP Theme Provider to take full control.
@himrocks33
Copy link
Author

Now also includes bug fixes for #53 and #49 as per @srivastavaanurag79 request.

@CarlosSLoureiro or @Thodor12 could you please review?

@Thodor12
Copy link
Contributor

Thodor12 commented Apr 4, 2024

With all the individual property additions, isn't it easier to just inherit the typings from the paper checkbox and using rest parameters to pass everything at once to the checkbox? (Whilst making sure to omit certain things we have to explicitly set)

Copy link
Contributor

@Thodor12 Thodor12 left a comment

Choose a reason for hiding this comment

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

Could I also get a before and after of how it looks, with pure basic configuration, no modified theming, etc.
If the UI changes it might have to be marked as a breaking change.

src/components/checkBox.tsx Show resolved Hide resolved
@himrocks33
Copy link
Author

@Thodor12 UI changes are minimal. Seems like you are grilling me for no reason on this PR. It’s pretty straightforward.

@srivastavaanurag79
Copy link
Owner

srivastavaanurag79 commented Apr 5, 2024

@himrocks33 Please add this feature to this merge request,i.e, limiter, a key name limit which limits the maximum number of items one can select in the multi-select.

**Future Features #59 **:

  1. Sectional List i.e. see below example
  • ASIA
    • UAE
    • INDIA
    • NEPAL
  • EUROPE
    • UK
    • SPAIN
    • FRANCE
  1. Chips style selected items (optional styling)
  2. Different View For Single Select, example below
    1_rQNkcjLw1RYtDZRNqoyQtw

@Thodor12
Copy link
Contributor

Thodor12 commented Apr 5, 2024

@Thodor12 UI changes are minimal. Seems like you are grilling me for no reason on this PR. It’s pretty straightforward.

If you think that that is your problem.

All I'm asking for is a simple before and after. I see a bunch of changes and I've got no idea of the impact of this.
If things start changing between versions we have to state it as a breaking change, otherwise people can get silently a new interface without knowing if they update the library.

@srivastavaanurag79
Copy link
Owner

It will be a breaking change since the checkbox style is being changed and some other new features are being added as well. The pull request will be strictly reviewed so please be patient and please be a bit polite everyone.

@srivastavaanurag79
Copy link
Owner

@Thodor12 If you have time please switch to @himrocks33 branch and run the project to review the changes and update it with your guidance if you find something unusual or not needed.

@himrocks33
Copy link
Author

I got pretty busy at work again for the time being. I feel like there is enough here for a feature release though. @srivastavaanurag79

@himrocks33
Copy link
Author

@srivastavaanurag79 #59 items seem like a complete rewrite almost to support the single select (which would probably be a whole new component really?) I doubt I have time for that right now. Bug Fixes and minor changes I can do. Where do we stand with getting this PR merged?

@srivastavaanurag79
Copy link
Owner

@himrocks33 #59 you Don't have to do them, those are future features

@srivastavaanurag79
Copy link
Owner

@himrocks33 if adding a limiter feature is possible please add it, a key name limit which limits the maximum number of items one can select in the multi-select, will try to review and merge this pull request this Sunday by myself if other contributors don't have time for the same.

@srivastavaanurag79
Copy link
Owner

I dont know why but disable property isn't working as it is supposed to be, it still lets me check/uncheck. and using Checkbox.Item has changed the view dramatically.

Sharing the screenshot, longer text are not visible properly and as you can select the label now, but the select area is restricted to the size of the label.
Screenshot 2024-04-18 at 12 31 29 PM

@srivastavaanurag79
Copy link
Owner

Fixed disable issue and checkbox label issue, select all issue
Screenshot 2024-04-18 at 2 53 21 PM

feat:
 limit?: number | null;
  limitError?: string;
  limitErrorStyle?: TextStyle;

limit the number of items one can select in multi select.
Copy link
Owner

@srivastavaanurag79 srivastavaanurag79 left a comment

Choose a reason for hiding this comment

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

I have reviewed all the changes. The major change was in the checkbox. Touchable Opacity was removed but it would not break any existing installed version. The code looks fine. I addressed the issues which I found.

@srivastavaanurag79 srivastavaanurag79 merged commit 7e29539 into srivastavaanurag79:master Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is library still actively maintained? Feature request: Disable Option Use Paper theme by default
3 participants