Add border on Title when focused #622
Conversation
| } | ||
|
|
||
| .blockHolderFocusedSemiBordered { | ||
| border-color: $gray-lighten-30; |
There was a problem hiding this comment.
Would it make sense to use css box-shadow to apply these focus states? In theory that should prevent any shifting, just applying a shadow on the outside edge of the element.
There was a problem hiding this comment.
Unfortunately box-shadow property is not available in React Native and as I investigated it is not a very straight forward change to replace borders with shadows in RN. Because Android and iOS have very different methods for doing it and both methods have different side effects that needs to be addressed.
iOS has these props:
https://facebook.github.io/react-native/docs/shadow-props#shadowcolor
Android has the elevation option but there are no other options to fine-tune the shadow effect. So I am not even sure it'll give the results we want.
https://facebook.github.io/react-native/docs/view-style-props#elevation
I tried to apply these quickly but it messed up the child views' styles in different ways, so it didn't look like an ideal last minute change, but, if you want we can open a separate issue for this and try later?
In this PR, I fixed the shifting of Android blocks. I don't expect any shifting will happen in cases where the block width is equal to window width, because in that case we don't have left&right borders at all.
There was a problem hiding this comment.
@pinarol - Code looks good! ✨
Just left a small comment there.
The title borders works well and the Android issue with horizontal borders is gone! 🎉
I did find a small issue with the title autofocus in both WPiOS and WPAndroid:
The title borders weren't added when the title was initially focused (opening an empty new post), but after tapping on the title, the borders would appear.
Let me know if you can't reproduce this issue.
Thanks for wrangling this!
|
@etoledom 👋 thanks for the review. I am going to address that issue about the empty post but before that I found another issue unfortunately, it looks a bit annoying and still couldn't come up with a solution. On Android, the title focus is getting lost in the first tap:
I am guessing the re-render of the title view in order to add the borders might be somehow triggering a blur event on Android side, I've seen the blur event coming from Android side but not sure about the root cause of it. This is not happening on iOS. Still looking into it. |
# Conflicts: # gutenberg # src/block-management/block-manager.js
…/gutenberg-mobile into issue/540-title-borders
Also border styling mechanism changes, we start just to play with border colors in order to provide smoother animation This also solves Android's re-render title issue on the first tap
pinarol
force-pushed
the
issue/540-title-borders
branch
from
683069f
to
6b23698
Compare
|
Hey @etoledom 👋 This is ready for another look. It looks like Android can somehow re-render the title if we add border to it and this makes a blur event get triggered. So I changed the way we add borders, I updated the container views with borders of transparent color. When we focus I just change the border color. So we will not only get rid of unnecessary re-renders but also we are removing possibilities of the shifting effects when we focus. This is how the shadow effect would work also, cc @iamthomasbishop
|
There was a problem hiding this comment.
Commented on WordPress/gutenberg#13970 (review)
cmd+c cmd+p:
@pinarol - This feels so much better! Great idea ✨
Also the code looks better organized and clean.
The issues mentioned before are gone and borders in general just work great.
I left a couple of small comments but I'm happy to ✅ and 
| placeholder={ __( 'Add title' ) } /> | ||
| </View> | ||
| <PostTitle | ||
| onRef={ ( ref ) => { |
There was a problem hiding this comment.
Nitpick: Can we name this ref instead of onRef? Since it seems to be more standard in React.
…rg-mobile into issue/609-Show-Title-block-when-HTML-mode * 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile: (44 commits) Updated bundles Add border on Title when focused (#622) Default to the device language for the example app Updates the gutenberg submodule ref. Updates the gutenberg submodule reference. Re-add rootTagsToEliminate option to RichText (#636) Give priority to the parent app translations Remove unnecessary import Fix lint issue Fallback to a locale without a regional code if not supported Add Greek support PR got merged upstream so, use upstream Run yarn i18n-cache on postinstall Run yarn prebundle before bundle:android and bundle:ios instead Use a forked+patched jsdom-jscore with normalization fix Revert "Update GB ref with fix for node.rel" Upload media progress bar is missing while media is uploading new (#631) Remove JS handling of title focus Update GB ref with fix for node.rel Update iOS translation values as arrays ...
Fix #540
Fix #551
This change adds borders to the Title when focused in order to match other blocks.
And also fixes a slight shift happening on Android when we focus to a block.
gutenberg PR needs to be merged first: WordPress/gutenberg#13970
To Test
Testing prerequisites
For WPiOS
Checkout the PRs branch to any arbitrary folder and cd .. to it
run yarn install, yarn start
Open XCode WPiOS on the latest develop
Clean build folder on Xcode, and then run the app
For WPAndroid
open grade.properties at WordPress-Android folder
add wp.BUILD_GUTENBERG_FROM_SOURCE = true to grade.properties
checkout the PRs branch in the subrepo of WordPress-Android repo
cd to WordPress-Android/libs/gutenberg-mobile
run yarn install, yarn start
yarn wpandroid on a separate terminal in the same directory
Test Steps:
WPiOS:
iPhone X and iPad special case:
WPAndroid: