-
Notifications
You must be signed in to change notification settings - Fork 741
Allow passing counter style to adjust margins of carousel component #3833
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
base: master
Are you sure you want to change the base?
Conversation
✅ PR Description Validation PassedAll required sections are properly filled out:
Your PR is good for review! 🚀 This validation ensures all sections from the PR template are properly filled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alwaysintune, I left a small comment about the naming convenience of the prop name.
Since it's only for the counter container, I think counterContainerStyle is better naming here and creates a clear separation between the container and the text styling.
Co-authored-by: Adi Mordo <adids1221@gmail.com>
Co-authored-by: Adi Mordo <adids1221@gmail.com>
Co-authored-by: Adi Mordo <adids1221@gmail.com>
| /** | ||
| * the counter's container style | ||
| */ | ||
| counterContainerStyle?: StyleProp<ViewStyle>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alwaysintune @adids1221
Generally, we should avoid adding styles for each view in the hierarchy.
Let's try to find a solution that solves the issue without an additional prop.
Please open a ticket if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the only solution to add margin to the right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To elaborate more - currently counter has an absolute position. Absolute position forces item design to conform, fill the full width, whilst carousel is written with use cases in mind, so a lot of props can't be used together (i. e. loop and item spacings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing with banners leads to cases where it would be best to have a different position for counter because it conflicts with dismiss x action
Description
Adds a new
counterContainerStyleprop to the Carousel component to allow customization of the counter container's styling. This complements the existingcounterTextStyleprop by enabling developers to adjust the counter container's appearance (margins, padding, positioning, background, etc.) whilecounterTextStylecontinues to control the text styling inside the counter.Changelog
Carousel - Added
counterContainerStyleprop (StyleProp) to customize the counter container's style. This allows adjusting margins, padding, positioning, and other visual properties of the counter container whenshowCounteris enabled.Additional info
This enhancement provides more flexibility for customizing the counter appearance, particularly useful for adjusting margins and positioning to match different design requirements or layouts.