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: backgroundColor apply refactoring #121

Merged
merged 3 commits into from
Feb 1, 2025
Merged

Conversation

sergeymild
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Jan 26, 2025

@sergeymild is attempting to deploy a commit to the Jovanni's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

codeclimate bot commented Jan 26, 2025

Code Climate has analyzed commit 25f9c6c and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Owner

@lodev09 lodev09 left a comment

Choose a reason for hiding this comment

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

Looks good. See minor comments

package.json Outdated Show resolved Hide resolved
@@ -250,7 +251,7 @@ export class TrueSheet extends PureComponent<TrueSheetProps, TrueSheetState> {
scrollableHandle={this.state.scrollableHandle}
sizes={sizes}
blurTint={blurTint}
background={backgroundColor}
background={(backgroundColor ? processColor(backgroundColor) : undefined) as any}
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe change the background type in TrueSheetNativeViewProps instead of casting to any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you change this property in TrueSheetNativeViewProps so that its type is number instead of ColorType, the user won't be able to use it as a regular color property. I don't see any problem with casting to any because it's part of the internal implementation and won't be exposed externally.

Copy link
Owner

Choose a reason for hiding this comment

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

the background prop is actually just a prop for the native component. The user facing prop is backgroundColor.

@lodev09 lodev09 mentioned this pull request Jan 26, 2025
Copy link

vercel bot commented Jan 27, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-true-sheet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 1, 2025 7:52pm

@@ -250,7 +251,7 @@ export class TrueSheet extends PureComponent<TrueSheetProps, TrueSheetState> {
scrollableHandle={this.state.scrollableHandle}
sizes={sizes}
blurTint={blurTint}
background={backgroundColor}
background={(backgroundColor ? processColor(backgroundColor) : undefined) as any}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
background={(backgroundColor ? processColor(backgroundColor) : undefined) as any}
background={(backgroundColor ? processColor(backgroundColor) : undefined)}

@@ -9,6 +9,7 @@ import {
type NativeSyntheticEvent,
type LayoutChangeEvent,
type ColorValue,
processColor,
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
processColor,
type ProcessedColorValue,

Copy link
Owner

Choose a reason for hiding this comment

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

Replace the type with this:

interface TrueSheetNativeViewProps
  extends Omit<TrueSheetProps, 'onPresent' | 'onSizeChange' | 'backgroundColor'> {
  contentHeight?: number
  footerHeight?: number
  background?: ProcessedColorValue | null
  scrollableHandle: number | null
  onPresent: (event: SizeChangeEvent) => void
  onSizeChange: (event: SizeChangeEvent) => void
  onContainerSizeChange: (event: ContainerSizeChangeEvent) => void
}

Copy link
Owner

@lodev09 lodev09 left a comment

Choose a reason for hiding this comment

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

going to merge this and update suggested code in main.
thanks @sergeymild

@lodev09 lodev09 merged commit e64de2e into lodev09:main Feb 1, 2025
5 checks passed
@lodev09
Copy link
Owner

lodev09 commented Feb 1, 2025

🚀 This pull request is included in v1.1.0. See Release 1.1.0 for release notes.

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