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

feat!: clamp sheetInitialDetent value #2370

Closed
wants to merge 2 commits into from

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Oct 2, 2024

Description

In case sheetInitialDetent value is not in range [0, sheetAllowedDetents.length - 1] I decided to clamp it.

Alternatives:

  • throw an error,
  • clamp it and print warning in dev mode that value has been clamped.

Open for discussion.

Test code and steps to reproduce

Test1649

Checklist

@kkafar kkafar requested review from alduzy and maciekstosio October 2, 2024 13:33
Copy link
Member

@alduzy alduzy left a comment

Choose a reason for hiding this comment

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

Good idea!
Let's remove the out of bounds errors error (edit: let's keep the one for iOS < 16 where the allowedDetents are replaced with system-defined ones) from ios/RNSScreen.mm then.

OR change them to warnings, which will give a clue about the misuse

@kkafar
Copy link
Member Author

kkafar commented Oct 2, 2024

@alduzy I think the checks on native side should stay as they are. This causes less coupling (I know we have a ton of assumptions on what our JS code does, but IMHO it should not legitimise us to create more coupling) and in case someone changes JS code in the future, there will be check for this invariant on native side.

@alduzy alduzy self-requested a review October 2, 2024 14:03
@kkafar
Copy link
Member Author

kkafar commented Oct 3, 2024

On meeting we decided to go with different way of error handling & different shape of API.

@kkafar kkafar closed this Oct 3, 2024
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.

2 participants