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

[RNMobile] Align Range Control (Android Slider component) to the left of it's container #25660

Merged

Conversation

jd-alexander
Copy link
Contributor

@jd-alexander jd-alexander commented Sep 26, 2020

Fixes wordpress-mobile/gutenberg-mobile#2667

Description

This fix simply moves to the slider to the left as much as possible so that there isn't a noticeable gap between the control and the left side of the screen.
Based on my investigation, the native implementation of the Slider control extends AppCompatSeekBar and it's within a container that has it's own margin/padding. I didn't see any style attributes that could be passed so that this could be left-aligned so if there's another solution that's more optimal it's more than welcomed.

How has this been tested?

I verified that the behavior has adjusted the Range Control on

  • Button
  • Columns
  • Cover
  • Latest Posts
  • Linear Gradient

To test on Android

  1. Add a Spacer component to the canvas.
  2. Click the settings button.
  3. Notice that the Slider control is more left-aligned.

To test on iOS

  1. Add a Spacer component to the canvas.
  2. Click the settings button.
  3. Notice that the Slider control is the same and no regression has occurred.

Screenshots

Left alignment of Slider

Before After

Thumb ripple effect of Slider

As seen in the above screenshots, the control could be moved a bit more to the left but I didn't do that because I started noticing that it would get cut off and the ripple effect of the draggable thumb was being cut.

Before After

Column Settings Slider

Below, I checked to ensure that this change did not impact the Columns Settings Slider usage. The main change is that there is a bit less space

Click to see screenshots
Before After

Button

Click to see screenshots
Before After

Cover

Click to see screenshots
Before After

Types of changes

This change involves creating a separate style for each platform and adding a margin-left for the Android style.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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.

@jd-alexander jd-alexander changed the title [RNMobile] Align Android Slider component to the left of it's container [RNMobile] Align Range Control (Android Slider component) to the left of it's container Sep 26, 2020
@etoledom etoledom self-requested a review October 1, 2020 08:12
@etoledom etoledom assigned etoledom and jd-alexander and unassigned etoledom Oct 1, 2020
@etoledom etoledom added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Oct 1, 2020
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

@jd-alexander - Thanks for this PR!

I tested on Google Pixel 3 XL simulator, and even though the Range Control is more to the left, it doesn't seem to be aligned yet with the title:

Screenshot 2020-10-01 at 10 41 40

@iamthomasbishop - What do you think?

@jd-alexander
Copy link
Contributor Author

Indeed @etoledom thanks for pointing this out. I am going to do another round with this one. Just a question, do you ever submit fixes to other open-source projects so we can pull in the updates if we can't fix something like this in our app? I know for one that sometimes they take a bit longer to do reviews. This is the Slider repo so I am thinking about that. In the past, I have just forked, and then included my fix as my own dependency but I am not sure if that's something we do. Let me know 👍

@etoledom
Copy link
Contributor

etoledom commented Oct 1, 2020

do you ever submit fixes to other open-source projects so we can pull in the updates if we can't fix something like this in our app?

Yes! We have some contributions on projects we use. What we have done is to send a PR to the original repo, and also patch locally to have the fix fast on our side while we wait for the update on the original.

@guarani guarani self-requested a review October 1, 2020 18:12
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

(I went ahead and added myself as a reviewer here since I was added to the gutenberg mobile PR).)

@jd-alexander this change works great on telephones (I tested the scenarios you outlined above), on tablet devices though the lack of perfect alignment (that you mentioned in the PR) is a bit more apparent.

Here's a screen recording as I comment in/out the -6px change you added. While it lines things up better, it still looks a bit unaligned (more noticeable here than on phone form factors).

I see your comment above about forking and fixing the control itself, is that something you want to pursue in this PR or leave it out?

@jd-alexander
Copy link
Contributor Author

Thanks for the tests and feedback here @guarani 🙏

I see your comment above about forking and fixing the control itself, is that something you want to pursue in this PR or leave it out?

I would probably pursue it in another PR. I am thinking to close this one. I just tried another fix that aligns the control using marginLeft of -16 as a layout prop which evaluates to 16dp on Android because I discovered that the Seekbar's internal style has that amount of padding to accommodate the thumb. So now the thumb is cut off as seen below.

So the solution to this would be a bit tricky as we would either have to disable the ripple effect as it would need some amount of padding to work or if we keep it that would mean changing the margin of the entire layout would need to be changed which isn't worth it for this improvement. Any other thoughts are welcome.

@guarani guarani self-requested a review October 2, 2020 21:27
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Hey @jd-alexander, sure I think it can be left out here as this PR makes a definite improvement in any case.
I noticed the styles were moved to the .js file in the latest commit — personally I think it's better to have them in the .scss like it was before, but this might be fine too so approving now ✅.

@jd-alexander
Copy link
Contributor Author

Hey @jd-alexander, sure I think it can be left out here as this PR makes a definite improvement in any case.

Thanks @guarani I will revert to the first solution.

I noticed the styles were moved to the .js file in the latest commit — personally I think it's better to have them in the .scss like it was before, but this might be fine too so approving now ✅.

Indeed! Thanks for this. I was trying this approach because I was wondering if the React Native platform does a transformation of these value for each device and I thought using a px value in the scss. was a bit more absolute. But yes, I will utilize the version that has the style present within the scss.But yes, another look at the documentation I realize most of the styles work just like css :)

@jd-alexander
Copy link
Contributor Author

@guarani I was looking at https://reactnative.dev/docs/height-and-width.html
and got confused by this line

All dimensions in React Native are unitless, and represent density-independent pixels.

😅

@guarani
Copy link
Contributor

guarani commented Oct 5, 2020

All dimensions in React Native are unitless, and represent density-independent pixels.

I've been thinking of them as similar to points on iOS, where a width of 1 point is 3 pixels wide on the newest devices (but was 1 pixel wide on older devices) — not sure if this is 100% correct on iOS, but it's what I've understood.

@jd-alexander jd-alexander force-pushed the rnmobile/android_align_slider_left branch from 530fbce to a497f25 Compare October 6, 2020 10:58
@iamthomasbishop
Copy link

@jd-alexander Thanks for jumping on this! Is the "cut off" in the example above happening because the container's overflow is hidden? I'm guessing the fix is not that straight-forward but thought I'd ask 😄 I think getting the gap closed as much as possible right now and refining in another PR would be fine.

This makes me wonder if (in the medium-to-long-term) we should re-structure how table cells are laid out so that cells are 100% of the sheet container by default and then titles/controls padded on the sides. Maybe it's not that straight-forward, but it would also allow us to do full-width separators without hacking negative margins 🤷 .

@jd-alexander
Copy link
Contributor Author

@jd-alexander Thanks for jumping on this! Is the "cut off" in the example above happening because the container's overflow is hidden? I'm guessing the fix is not that straight-forward but thought I'd ask 😄 I think getting the gap closed as much as possible right now and refining in another PR would be fine.

Thanks for chiming in @iamthomasbishop the cut off is due to how the native Android component was designed. So yes, you are correct that the fix isn't as straight forward as the native container has it's own padding to support the ripple effect of the thumb.

This makes me wonder if (in the medium-to-long-term) we should re-structure how table cells are laid out so that cells are 100% of the sheet container by default and then titles/controls padded on the sides. Maybe it's not that straight-forward, but it would also allow us to do full-width separators without hacking negative margins 🤷 .

Yes, if this were the case then we could solve this problem easily so until then we will just move the control over a bit and leave this issue open for the future.

@jd-alexander
Copy link
Contributor Author

@guarani can you get this merged in for me when you get a chance. Thank you 🙇

@guarani guarani merged commit 15f4799 into WordPress:master Oct 12, 2020
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 12, 2020
@guarani
Copy link
Contributor

guarani commented Oct 12, 2020

Merged 🎉

@jd-alexander jd-alexander deleted the rnmobile/android_align_slider_left branch October 12, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] Align slider with the content on the left
4 participants