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

Extract Gallery component for semi-cross-platform Gallery block #18265

Merged
merged 36 commits into from
Dec 6, 2019

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented Nov 4, 2019

Description

This PR extracts a Gallery component for the semi-cross-platform Gallery block. This will allow for web and mobile to share a common edit.js file, which will in turn use the respective web and mobile implementations of the Gallery component.

PR Hierarchy

This PR is part of the PR hierarchy described here. This PR can be used to test the aggregate changeset and integration of components within the related PRs.

Note:
This PR will need occasional rebasing in order to test the integration with the mobile components in downstream PRs.

Changes

  • Gallery component is extracted to a separate file (the block should render the same for web)
  • It uses the Platform module to provide different instructions for adding images on mobile
  • It adds an isMobile prop via the withViewportMatch HOC
  • It adds a mobile-specific icon prop to RangeControl

The icon prop will be hidden for web after the cross-platform PR for RangeControl is merged (without that merged, icon will be passed through as an attribute to the underlying <input> element). So this PR depends on the RangeControl PR being merged first.

BlockIcon:

I've extracted the block icon into a shared-icon file (with web and mobile variants) so that we can apply dark-mode styles on mobile. Also, although the icons use the same underlying svg, they are wrapped differently. Web styles / behaviors should be preserved in this PR.

Separator type:

To make separator indentation consistent for controls in the bottomsheet on mobile, it is currently necessary to add separatorType="fullWidth" to the controls: https://github.com/WordPress/gutenberg/pull/18265/files#diff-34f83402fe8954f15f8d404d01c9e4c9R302. This can "leak" into the web elements as an attribute as described here: #18155 (comment). I didn't notice any changes in behavior (exposed to the user), but I saw the attribute appearing in the elements. Any guidance on whether this is a blocker for web side is appreciated.

This PR should not result in any change in behavior for the web version of Gallery block.

To test

Web:

Checkout this PR and test locally (npm run env update, npm run dev) and ensure no behavior has changed on web for the Gallery block.

Also, all associated automated tests should pass.

Mobile:

Related PRs can be tested via this PR.

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.

@mkevins mkevins added [Status] In Progress Tracking issues with work in progress Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Block] Gallery Affects the Gallery Block - used to display groups of images labels Nov 4, 2019
@mkevins mkevins mentioned this pull request Nov 5, 2019
@mkevins mkevins force-pushed the try/gallery-draft-extract-gallery branch from 2a6da85 to 3e5f2ad Compare November 7, 2019 02:32
@mkevins mkevins changed the base branch from master to try/gallery-draft-add-native-gallery November 7, 2019 02:33
@mkevins mkevins force-pushed the try/gallery-draft-add-native-gallery branch from 6b4cf2c to 3edb3eb Compare November 12, 2019 13:04
@mkevins mkevins force-pushed the try/gallery-draft-extract-gallery branch from cbd2370 to 3effb45 Compare November 12, 2019 13:05
@mkevins mkevins force-pushed the try/gallery-draft-add-native-gallery branch from 3edb3eb to 4190b97 Compare November 19, 2019 13:35
@mkevins mkevins force-pushed the try/gallery-draft-extract-gallery branch 2 times, most recently from e26655a to 00e5d51 Compare November 19, 2019 13:50
@mkevins mkevins force-pushed the try/gallery-draft-add-native-gallery branch from 4190b97 to 8b1e8b3 Compare November 19, 2019 13:58
@mkevins mkevins force-pushed the try/gallery-draft-extract-gallery branch from 00e5d51 to 2eac8cb Compare November 20, 2019 06:14
@mkevins mkevins force-pushed the try/gallery-draft-add-native-gallery branch from ac55dd8 to 85b45af Compare November 21, 2019 06:19
@mkevins mkevins force-pushed the try/gallery-draft-extract-gallery branch from 2eac8cb to 7ca3228 Compare November 21, 2019 06:20
@mkevins mkevins force-pushed the try/gallery-draft-add-native-gallery branch from 85b45af to 1e403ff Compare November 22, 2019 06:00
@mkevins mkevins force-pushed the try/gallery-draft-extract-gallery branch from 7ca3228 to f99f853 Compare November 22, 2019 06:00
@mkevins mkevins force-pushed the try/gallery-draft-add-native-gallery branch from 1e403ff to 7a3fa58 Compare November 25, 2019 03:41
@mkevins mkevins force-pushed the try/gallery-draft-extract-gallery branch from f99f853 to 5a12cc2 Compare November 25, 2019 03:42
@mkevins mkevins force-pushed the try/gallery-draft-add-native-gallery branch from 7a3fa58 to f030539 Compare November 26, 2019 06:51
@mkevins mkevins force-pushed the try/gallery-draft-extract-gallery branch from 396dc3c to d700b2b Compare November 26, 2019 06:51
@mkevins mkevins marked this pull request as ready for review November 27, 2019 12:44
@mkevins mkevins force-pushed the try/gallery-draft-extract-gallery branch from 66670c8 to 6f7d35f Compare December 5, 2019 02:30
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Things worked well in my tests, and I did not found any regression!
I guess we should try to improve and reduce the needs for platform checks/platform-specific props, but for now, I think this solution is a good step to increase reusability between mobile and web.
Nice work @mkevins 👍

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

@mkevins This is looking good overall, I just entered 2 comments. I want to make a last test tomorrow. Could you update PR branches so that we can have the latest Slider changes as well? After that hopefully we'll be good to go.

packages/block-library/src/gallery/edit.js Outdated Show resolved Hide resolved
Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

LGTM! Great work! 🎉

Tested with steps described here.

@pinarol pinarol merged commit 66ea490 into master Dec 6, 2019
@pinarol pinarol deleted the try/gallery-draft-extract-gallery branch December 6, 2019 13:17
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images 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.

4 participants