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

[Button Block]: Add native mobile support for padding (WIP) #32218

Closed

Conversation

stacimc
Copy link
Contributor

@stacimc stacimc commented May 25, 2021

Description

WIP to add native mobile support for button padding. For ease of testing this PR includes changes to add the block support to the Button block (from #31774)

There were a number of issues with adding native support for padding:

  1. The spacing block support uses a BoxControl input to allow setting padding for individual sides of the button. This component is not supported natively.
  2. Because it is possible to set different padding selections per-side on web, the native controls must also allow independently setting padding per side.
  3. The existing native Button is implemented as a class component and must be refactored in order to use the existing utilities fro converting units for mobile.
  4. The existing UnitControl, which I use here as a substitute for the BoxControl, seems to be lacking some native support -- specifically, the value and unit have to be handled separately. This vastly complicates the logic needed in an implementation of something like BoxControl.

This PR:

  • Refactors Button into a functional component
  • Pulls the controls out into a separate file
  • Adds a very rough first pass at padding support

The logic in this first pass is overly complicated and essentially re-implements a small subset of the functionality of BoxControl. I think the correct approach from here is to abandon this work and focus on getting full native support for BoxControl.

Major Known Issues:

  • The UI does not match web and is clunky/much too large.
  • There is no support for changing all padding values at once, or just top/bottom left/right (they must be individually updated)
  • Padding values are not visually correct, although the correct styles are being applied, and only bottom padding updates. When testing I found that applying a padding style to the button works, but paddingTop for instance does not. Using margin also works.
  • The button loads in slowly on Android; the refactor needs to be analyzed for its effect on performance.
  • Padding causes issues when width is set on the button
  • The + and - controls on the UnitControl are very buggy ( eg if you click twice it goes from 1 to 11 )

How has this been tested?

Manually. Because this PR refactors the button, all functionality must be tested.

Screenshots

Screen Shot 2021-05-27 at 1 36 10 PM

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@stacimc stacimc changed the title WIP Add native mobile support for Button padding [Button Block]: Add native mobile support for padding (WIP) May 27, 2021
@stacimc stacimc force-pushed the add/padding-support-native-button branch from 6bc74c5 to 584853c Compare May 27, 2021 20:58
@stacimc
Copy link
Contributor Author

stacimc commented Jun 4, 2021

Closing this PR until BoxControl is supported natively.

@stacimc stacimc closed this Jun 4, 2021
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