-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
Removed left padding on KeyboardToolbar when navigation arrows are hidden #851
base: main
Are you sure you want to change the base?
Conversation
paddingLeft: showArrows ? 8 : 0, | ||
paddingRight: 8, |
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.
My current idea was to add View
instead of <>
for button group and apply a style paddingLeft
there.
And accordingly I wanted to remove paddingHorizontal
from container and add paddingRight: 8
to doneButtonContainer
. In this case we will not need to write additional conditional code 🤔
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.
Can you try to make these changes and I'll run CI to see if it passes or not? 👀
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 was exactly my first idea as well!
What I think might be an even better approach is the following:
- Remove
paddingHorizontal: 8
fromtoolbar
style - Add a
View
instead of<>
and give it the following styles:
arrowsButton: {
flexDirection: 'row',
gap: 5,
paddingHorizontal:8,
},
- Remove the done
doneButtonContainer
styles - add
paddingHorizonal: 8
todoneButton
- in Arrow.tsx remove
marginHorizontal: 5
fromarrowUpContainer
This way both done and arrow buttons have a padding of 8 and the 2 arrow buttons still have the gap of 5 between them. Also with this if the arrow or done button gets removed the whole container is removed as well.
Also in the previous version the done button didnt have padding on the left only on the right (margin). Now it is centered in its container.
@@ -38,6 +38,7 @@ module.exports = { | |||
{ | |||
quoteProps: "consistent", | |||
trailingComma: "all", | |||
endOfLine: "auto", |
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.
Had to add this otherwise every single line would error out for me.
📊 Package size report
|
}, | ||
arrowsButton: { | ||
flexDirection: "row", | ||
gap: 5, |
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.
I think gap
was added relatively recently and I support older RN versions as well, so it's better to use paddings/margins to make sure all library users have consistent experience 😊
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.
Makes sense! I remove the gap
and added marginRight: 2.5
and marginLeft: 2.5
on the arrowUp/down containers respectively. I also made it so the arrowContainer is a base container and the arrowUp/down add styles to the base container instead of the down container adding styles to the up container.
alignItems: "center", | ||
justifyContent: "center", |
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.
Why it's needed now? 🤔
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.
Ah you are right, thats not needed.
}, | ||
doneButtonContainer: { | ||
marginRight: 8, | ||
paddingHorizontal: DEFAULT_BUTTON_PADDING, |
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.
With a new approach arrow both done/arrows will have a horizontal padding, right?
I think with a previous approach only arrows had paddingRight
and theoretically long content could overlap with "Done" button (since Done buttong didn't have any margin/padding from left). I think a new approach makes sense 👍
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.
yes with this approach both containers will have the exact amount of horizontal padding.
I tested it with long content (a custom toolbar) and there is no overlapping in this approach with or without the arrows/done button!
@spaansba after your changes button have different positioning: This component is a pretty precise replica of iOS toolbar and I'd like to continue to have pixel-perfect UI 🙂 |
interesting, Ill have a look |
📜 Description
Remove padding when showArrows={false} on KeyboardToolbar
The KeyboardToolbar component currently applies a fixed paddingHorizontal of 8px even when showArrows is set to false. This creates an unnecessary left padding gap when arrows are hidden. This PR modifies the toolbar styling to conditionally apply paddingLeft only when arrows are visible.
💡 Motivation and Context
When setting showArrows={false}, users expect the whole container to be gone.
Closes #848
📢 Changelog
JS
🤔 How Has This Been Tested?
Tested manually with both showArrows={true} and showArrows={false} to ensure:
📸 Screenshots (if appropriate):
Before:

After:

📝 Checklist