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] Column block #19013

Merged
merged 231 commits into from
Apr 8, 2020
Merged

[RNMobile] Column block #19013

merged 231 commits into from
Apr 8, 2020

Conversation

jbinda
Copy link
Contributor

@jbinda jbinda commented Dec 9, 2019

Description

Brings Column Block feature to mobile version.

Merge it first gutenberg PR:
[RNMobile] Inner Block Navigate down through hierarchy
[RNMobile] fix group appender behaviour
edited:
[RNMobile]Create cross platform useResizeObserver() hook
[RNMobile] block-wrapper for mobile version

[RNMobile] Add useResizeObserver() to Column Block

[RNMobile] Adjustments in Column for bordering refactor
[RNMobile] Include support inserter in Column Block❓ 🚧

Please also refer to:
Related gutenberg-mobile PR

And also comments about design below:
comment1
comment2
comment3
breakpoints comment

How has this been tested?

You can test production builds:
iOS: IPA 25912
Andorid: APK 42598

Or try with demo app with below steps:

  1. Add below code in initial-html
/**
 * @format
 * @flow
 */

export default `
<!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:heading -->
<h2>Test Heading</h2>
<!-- /wp:heading --></div></div>
<!-- /wp:group -->
 
<!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"></div></div>
<!-- /wp:group -->
 
<!-- wp:columns -->
<div class="wp-block-columns"><!-- wp:column -->
<div class="wp-block-column"></div>
<!-- /wp:column -->
 
<!-- wp:column -->
<div class="wp-block-column"><!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image --></div></div>
<!-- /wp:group --></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->


`;
  1. Try to play with column block
  • add/remove Column using Stepper in bottom sheet settings
  • add Column using Appender
  • add Column using Inserter
  • remove Column using remove mobile toolbar button
  • change Column order
  • change Column verticalAlignment (for all Column from Columns block and individually for each Column)
  • rotate device between horizontal and vertical position
  • nesting Columns and Group inside Column (keep notice on this one)
  1. Do the same starting with empty initial-html

Screenshots

Types of changes

New feature ( Columns block )

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. .

jbinda and others added 30 commits October 8, 2019 13:36
WIP: add hierarchy of click-down

WIP: add hierarchy of click other element same parent

EOD: click-down logic

EOD: click-down logic fix

WIP: dim unfocus element

WIP: add navigate up button on floating toolbar

EOD: change color of FloatingToolbar arrow to white

change focus border color from gray to blue

EOD: click-down logic

lock onFocus on AztecView when block is not selected

change dashed border color

apply style to keep same innerblock size

work with styles

work with styles

adjust borders margin

pass isParentSelected to RichText to unlock onClick event
@pinarol
Copy link
Contributor

pinarol commented Apr 7, 2020

@jbinda hi 👋 I repeated the tests, this time on iOS, they seem to work just fine.

But we have a couple of accessibility problems.

  1. The button icons doesn't seem right on RTL mode:

As you see "test" is inside the first Column, and the button directions(icons) should be the opposite:

Screen Shot 2020-04-07 at 20 18 38

I chose this on XCode to be able to test RTL:

Screenshot 2020-04-07 at 19 10 58

  1. The VoiceOver doesn't seem to act right. I am not able to tap into the mover buttons for example. The focus of VoiceOver stays in the "Columns" block level. Is there something I am doing wrong maybe? Are you able to tap on the mover buttons using VoiceOver?

cc @etoledom

@pinarol
Copy link
Contributor

pinarol commented Apr 7, 2020

BTW, I checked the code as well, I think it looks pretty good. 🎉 Thank you for the updates. It'd be awesome if these accessibility issues are fixed as well. That way we'd just remove the DEV flags within this PR.

@jbinda
Copy link
Contributor Author

jbinda commented Apr 8, 2020

The VoiceOver doesn't seem to act right. I am not able to tap into the mover buttons for example. The focus of VoiceOver stays in the "Columns" block level. Is there something I am doing wrong maybe? Are you able to tap on the mover buttons using VoiceOver?

If you have tried it on production build it still has old translations code and that is one of the cause why it do not work. If you test it another way I can try to replicate and try to see where is the issue

The button icons doesn't seem right on RTL mode:

Ok it seems one more flag is necessary to change the direction of arrows if RTL mode is on. I will fix that today

BTW, I checked the code as well, I think it looks pretty good. 🎉 Thank you for the updates. It'd be awesome if these accessibility issues are fixed as well. That way we'd just remove the DEV flags within this PR.

I will try to fix a11y today.

Also if we want to remove dev flag right away I will prepare production build with final version.

@pinarol
Copy link
Contributor

pinarol commented Apr 8, 2020

If you have tried it on production build it still has old translations code and that is one of the cause why it do not work. If you test it another way I can try to replicate and try to see where is the issue

I tried it on DEV with the latest changes. I recorded this video. Even if I try tapping on mover buttons it is not letting me and reading the "Columns" block description as if buttons are not accessible.

@pinarol
Copy link
Contributor

pinarol commented Apr 8, 2020

One thing I know: "If a parent view is accessible, its children won't be seen by VoiceOver."

We have this code piece /block-list/block.native.js

			<TouchableWithoutFeedback
				onPress={ this.onFocus }
				accessible={ ! isSelected }
				accessibilityRole={ 'button' }
			>

isSelected is false for Columns if I focus on any inner block and this is expected. But this is making accessible={ true } on Columns. It seems to me like we should add one more check:

accessible={ ! ( isSelected || isChildBlockSelected ) }

@pinarol
Copy link
Contributor

pinarol commented Apr 8, 2020

After I tried those changes in my local I am able to hear VoiceOver on mover buttons and issue is fixed. 🎉🎉🎉 I'll send you the patch.

But now I recognize that some accessibility texts are set wrongly.

In portrait mode:
Tapping Up button says: "Move block right ..."

In landscape mode:
Tapping Right button says: "Move block up ..."

@jbinda
Copy link
Contributor Author

jbinda commented Apr 8, 2020

I have applied fixes for a11y for VoiceOver as well as for messages and arrow directions in RTL

@pinarol
Copy link
Contributor

pinarol commented Apr 8, 2020

Issues about RTL labels and VoiceOver are fixed! 🎉🎉🎉

  • RTL support
  • VoiceOver support
  • VoiceOver support in RTL mode

I think this is ready to go!

We can remove DEV flags and :shipit:

Great work!

@jbinda
Copy link
Contributor Author

jbinda commented Apr 8, 2020

I removed the devOnly flag and after CI passed will merge :)

@jbinda
Copy link
Contributor Author

jbinda commented Apr 8, 2020

merged ! 🎉 🎉 🚀

@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 8, 2020
@pinarol pinarol deleted the rnmobile/column-block branch April 9, 2020 08:57
@jbinda
Copy link
Contributor Author

jbinda commented May 5, 2020

1. Background

Columns/Column block on mobile has very similar code structure to the web version. However some slight differences were needed to make it work on mobile.
The architecture is hooks based and looks like below:

  • ColumnsEdit component is exported
  • It tooks some props from store and renders ColumnsEditContainerWrapper which attach withDispatch to ColumnsEditContainer and inject actions that can be called there
  • ColumnsEditContainer renders InspectorControls, InnerBlocks, resizeListener and apply proper styling to whole block

2. Current state (features included)

  • useResizeObserver() hook to calculate Columns container size
  • calculate Columns in row number based on container size and predefined BREAKPOINTS (1)
  • wrap Columns depending of its number and calculated columns in row number
  • rotation of Movers buttons depending on Columns layout
  • stretch neighbours Column to fit height to Column with highest content (2)
  • verticalAlignment setting - can be set globally for each Column child via Columns block settings or individually for each Column
  • delete Columns block when try to delete last Column inside
  • adding Column via Inserter or via Appender - new Column has alignment option set in Column block
  • adding/removing Column via Stepper in Columns setting
  • minimum number of columns is 1 (deleting the last one will delete the whole Columns block). There is no maximum Column limit that user can add on mobile (3)

(n) - see point n in next section

3. Things to be aware of

  1. In current restriction of max content width equals 580px and applied BREAKPOINTS there will be no more than 2 Column in each row even on larger mobile screen (e.g. iPad). However the current logic will handle the case without above limit - but keep in mind putting to many Column in one row will break the layout so probably some minimum width limitation should be applied
  2. To allow stretch Column placed side-by-side I need to modify the InnerBlock API and pass some styling from Columns block
  • __experimentalMoverDirection - defines if Movers should be in horizontal or standard vertical position (also it includes RTL support)
    default: “vertical”
    values: “horizontal”, “vertical”
  • contentStyle - this props goes to each Column child of Columns block and set calculated Column width depending on Columns container size, number of Column in row and device orientation. Setting this width is necessary to wrap the Column to next row. Also flexWrap: wrap style needs to be applied in BlockList component (it is achieved by passes horizontal={true} prop described below)
    default: undefined,
    values: any object with style props
  • contentResizeMode="stretch" - this prop is used in BlockList component in renderItem() to pass {flex: 1} style to ReadableContentView hich is also necessary to achieve stretch to content
    default: undefined,
    values: “stretch”
  • horizontal={ true } - this prop do three things. One is to set proper style in BlockList. Second is to set proper direction in FlatList rendered under the hood of BlockList component. The third one is to set proper contentContainerStyle on FlatList to allow the whole content to wrap
    default: false,
    values: true, false
  1. Layout does not break on mobile when you add many child Column because of the wrapping logic - however avoid maximum Column limitation can cause some issue on web side. AFAIK on web version some approach on the limitation on Column number is ongoing. At this moment web version has no inserter support and the only way to add Column is using Slider in options - it is limited to maximum number of 6. We skip that limitation on mobile implementation because of issue to handle it properly.

4. Needs to be addressed/considered

@pinarol
Copy link
Contributor

pinarol commented May 5, 2020

Thank you @jbinda for dropping that summary!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants