Skip to content

Removed left padding on KeyboardToolbar when navigation arrows are hidden#851

Closed
spaansba wants to merge 4 commits intokirillzyusko:mainfrom
spaansba:remove-left-padding
Closed

Removed left padding on KeyboardToolbar when navigation arrows are hidden#851
spaansba wants to merge 4 commits intokirillzyusko:mainfrom
spaansba:remove-left-padding

Conversation

@spaansba
Copy link
Copy Markdown

@spaansba spaansba commented Mar 11, 2025

📜 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

  • Removed paddingHorizontal from toolbar styles
  • Added conditional paddingLeft based on showArrows prop
  • Added paddingRight to maintain right-side spacing
  • Added showArrows to toolbarStyle useMemo dependencies

🤔 How Has This Been Tested?

Tested manually with both showArrows={true} and showArrows={false} to ensure:

  • With arrows visible, padding is properly applied on both sides (8px)
  • With arrows hidden, left padding is removed (0px) while right padding is maintained (8px)

📸 Screenshots (if appropriate):

Before:
Before

After:
After

📝 Checklist

  • CI successfully passed
  • I added new mocks and corresponding unit-tests if library API was changed

Comment on lines +112 to +113
paddingLeft: showArrows ? 8 : 0,
paddingRight: 8,
Copy link
Copy Markdown
Owner

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 🤔

Copy link
Copy Markdown
Owner

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? 👀

Copy link
Copy Markdown
Author

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 from toolbar 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 to doneButton
  • in Arrow.tsx remove marginHorizontal: 5 from arrowUpContainer

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.

Comment thread .eslintrc.cjs
{
quoteProps: "consistent",
trailingComma: "all",
endOfLine: "auto",
Copy link
Copy Markdown
Author

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 12, 2025

📊 Package size report

Current size Target Size Difference
169971 bytes 169925 bytes 46 bytes 📈

},
arrowsButton: {
flexDirection: "row",
gap: 5,
Copy link
Copy Markdown
Owner

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 😊

Copy link
Copy Markdown
Author

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.

Comment on lines +232 to +233
alignItems: "center",
justifyContent: "center",
Copy link
Copy Markdown
Owner

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? 🤔

Copy link
Copy Markdown
Author

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,
Copy link
Copy Markdown
Owner

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 👍

Copy link
Copy Markdown
Author

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!

@kirillzyusko
Copy link
Copy Markdown
Owner

@spaansba after your changes button have different positioning:

ToolbarFirstInputFocused-diff

This component is a pretty precise replica of iOS toolbar and I'd like to continue to have pixel-perfect UI 🙂

@spaansba
Copy link
Copy Markdown
Author

interesting, Ill have a look

@kirillzyusko
Copy link
Copy Markdown
Owner

Closing in favour of #865

kirillzyusko added a commit that referenced this pull request Mar 17, 2025
## 📜 Description

Apply arrow/done paddings on elements level, not entire container, so
that conditional rendering will exclude potentially undesired paddings.

## 💡 Motivation and Context

Potentially "padding by default" approach may be not desired when you
want your custom element (content) to place that space.

So in this PR I re-worked an approach, and if done/arrows are hidden,
then `content` will fill entire container.

Closes
#848

Successor of
#851

## 📢 Changelog

<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->

### JS

- removed `paddingHorizontal` from main container;
- added container for arrows and apply padding there;
- apply padding (+8px) to Done button (to compensate removed padding
from main container);

## 🤔 How Has This Been Tested?

Tested manually on iPhone 15 Pro (iOS 17.5 Fabric).

## 📸 Screenshots (if appropriate):

|No arrows|No done button|No toolbar elements|All elements|
|----------|----------------|--------------------|------------|

|![image](https://github.com/user-attachments/assets/7062f6cb-cad4-4189-a9a2-f431f7cc729f)|![image](https://github.com/user-attachments/assets/e6a0602a-2b03-4a8a-af6f-eba9c42822ba)|![image](https://github.com/user-attachments/assets/04f1d1a5-7b29-439c-a01d-f7aea92a94f5)|![image](https://github.com/user-attachments/assets/eb88dc89-20e2-418b-8268-7b3f8d8046a3)|

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
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.

KeyboardToolbar still shows container of arrows even when showArrows={false}

2 participants