[RNMobile] Gallery - Button #18264

mkevins · 2019-11-04T11:03:40Z

Description

This PR introduces a Button component for the semi-cross-platform Gallery block. The purpose of this component is to provide UI buttons that can be styled for the native implementation of the Gallery block.

This Button component is used by the GalleryImage component introduced here: #18155. The web implementation of the Gallery block uses IconButton, which, in turn, uses Button from @wordpress/components. Initially, I attempted to use the mobile implementation of IconButton, and then Button, but I struggled to find a way to make this work. Our mobile implementation of the Button component seemingly lacks a way to style the button for the purpose of the gallery image.

Ideally, Button (or IconButton) from @wordpress/components can be general enough to allow re-use from the GalleryImage component, supporting the respective designs, but perhaps it's best to constrain the scope here for the first iteration of mobile Gallery block, as the potential for reuse of Button may require a wider set of changes.

The changeset here is used in the related "parent" PR: #18111, which includes these changes integrated with related changes in other components necessary for the gallery block.

To test

Test this component via the main draft 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.

pinarol · 2019-11-05T08:28:57Z

packages/block-library/src/gallery/gallery-button.native.js

+			disabled={ isDisabled }
+		>
+			<View style={ buttonStyle }>
+				{ isString( icon ) ?


isString check is done inside Icon component, it shouldn't be necessary here, in theory...

You're quite right about that 😄 thanks for pointing it out. I've removed the isString check.

pinarol · 2019-11-05T08:50:54Z

This Button component is used by the GalleryImage component introduced here: #18155. The web implementation of the Gallery block uses IconButton, which, in turn, uses Button from @wordpress/components. Initially, I attempted to use the mobile implementation of IconButton, and then Button, but I struggled to find a way to make this work. Our mobile implementation of the Button component seemingly lacks a way to style the button for the purpose of the gallery image.

Right, I think we need more props for Icon positioning/size and we need to allow overriding styles for active and disabled states separately. I believe this can be investigated as a part of wordpress-mobile/gutenberg-mobile#1411 Currently web is using CSS to override button styles but we don't have a way to do the same. It is tough to re-design a component from @wordpress/components inside another PR, I believe it deserves its own PR. And it'd be really nice to handle this in a full x-platform way, so I am hopeful about doing this as a part of wordpress-mobile/gutenberg-mobile#1411

mkevins · 2019-11-06T03:36:56Z

Hi Pinar, thanks for taking a look at this one.

It is tough to re-design a component from @wordpress/components inside another PR, I believe it deserves its own PR.

I think that makes a lot of sense, especially if we are aiming for a general cross-platform solution there. Thanks for linking to the issue tracking the cross-platform development for Button.

For the scope of this PR (and the related native Gallery work), is it acceptable to have this separate implementation of Button, i.e. defer the necessary modifications to the mobile implementation within @wordpress/components, and address code reuse in a later Gallery PR when the cross-platform Button is ready?

pinarol · 2019-11-06T07:06:52Z

For the scope of this PR (and the related native Gallery work), is it acceptable to have this separate implementation of Button, i.e. defer the necessary modifications to the mobile implementation within @wordpress/components, and address code reuse in a later Gallery PR when the cross-platform Button is ready?

I think cross-platform Button component will cover this anyway so I just want to avoid re-work, and we want to make it happen not later than ~2 months. Could you list what kind of changes needs to be done in @wordpress/components/Button for Gallery to be able to use it?

mkevins · 2019-11-06T09:05:07Z

Merged into GalleryImage (which depends on Button). Tracked here: #18155

mkevins · 2019-11-06T09:07:30Z

Could you list what kind of changes needs to be done in @wordpress/components/Button for Gallery to be able to use it?

I'll open an issue for that, and link it here: wordpress-mobile/gutenberg-mobile#1411.

* WIP - add gallery button * Fix lint errors * Remove isString check from gallery button

* Add native gallery - gallery image * WIP - refactor native gallery image * WIP - gallery image - deselect caption when image tapped * Add MediaUploadProgress to GalleryImage * Extract styles from gallery image * Add failed upload UI to gallery image * Remove HOC from gallery image * Remove unused props / methods in gallery image * Remove gallery image when upload is cancelled * Remove unused code from gallery image * Use accessibilityLabel prop on gallery image buttons * Remove href code from gallery image * Extract remaining styles from gallery image * Fix lint errors * [RNMobile] Gallery - Button (#18264) * WIP - add gallery button * Fix lint errors * Remove isString check from gallery button * Fix isCropped style for mobile gallery image * Accept style override props in RichText CAPTION - Fix lint * Add colorScheme styles for gallery image container * Add buttonActive style for gallery image * Clean up button styles on gallery image * Fix lint * Fix lint * Use withPreferredColorScheme in gallery image * Fix lint * Add some caption styles to gallery image * Use withPreferredColorScheme HOC for gallery caption * Refactor GalleryImage styles * Condense gallery changes to native RichText component * Extract gallery caption type flags for readability * Use RichText force blur prop in GalleryImage * Add ellipsis mode and ScrollView to gallery caption * Fix lint * Remove unnecessary View from gallery button * Adjust gallery button icon styles * Center align gallery caption text * Adjust gallery caption font-size to 12px * Use RichText to display unselected GalleryImage caption * Use child-first approach for gallery image UI components * Fix lint * Use full height for gallery caption scroll view * Use transparent background by default for gallery caption * Eliminate <p> tags in gallery image caption * Use variables instead of functions in gallery image scss * Extract gallery button icon sizes to constants * Tidy up gallery image style constants * Fix scss imports for jest * Use "p" tagName in gallery image on iOS to fix alignment

mkevins added 2 commits November 4, 2019 20:41

WIP - add gallery button

b7fe6a7

Fix lint errors

216c78b

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 changed the title ~~[RNMobile] Gallery - Buttons~~ [RNMobile] Gallery - Button Nov 5, 2019

This was referenced Nov 5, 2019

[RNMobile] Gallery - GalleryImage component #18155

Merged

Mobile gallery draft #18111

Closed

pinarol reviewed Nov 5, 2019

View reviewed changes

Remove isString check from gallery button

5cdac2b

mkevins changed the base branch from master to try/gallery-draft-gallery-image November 6, 2019 08:36

mkevins marked this pull request as ready for review November 6, 2019 08:52

mkevins removed the [Status] In Progress Tracking issues with work in progress label Nov 6, 2019

mkevins merged commit 85be21b into try/gallery-draft-gallery-image Nov 6, 2019

mkevins deleted the try/gallery-draft-buttons branch November 6, 2019 09:03

mkevins mentioned this pull request Nov 7, 2019

Gallery block - semi cross platform approach wordpress-mobile/gutenberg-mobile#1416

Closed

3 tasks

mkevins added a commit that referenced this pull request Nov 19, 2019

[RNMobile] Gallery - Button (#18264)

cb23133

* WIP - add gallery button * Fix lint errors * Remove isString check from gallery button

mkevins added a commit that referenced this pull request Nov 21, 2019

[RNMobile] Gallery - Button (#18264)

cc59b10

* WIP - add gallery button * Fix lint errors * Remove isString check from gallery button

mkevins added a commit that referenced this pull request Nov 25, 2019

[RNMobile] Gallery - Button (#18264)

91427be

* WIP - add gallery button * Fix lint errors * Remove isString check from gallery button

mkevins added a commit that referenced this pull request Nov 26, 2019

[RNMobile] Gallery - Button (#18264)

62f06c4

* WIP - add gallery button * Fix lint errors * Remove isString check from gallery button

mkevins added a commit that referenced this pull request Nov 28, 2019

[RNMobile] Gallery - Button (#18264)

2ab4807

* WIP - add gallery button * Fix lint errors * Remove isString check from gallery button

mkevins added a commit that referenced this pull request Dec 3, 2019

[RNMobile] Gallery - Button (#18264)

c4f726b

* WIP - add gallery button * Fix lint errors * Remove isString check from gallery button

mkevins added a commit that referenced this pull request Dec 4, 2019

[RNMobile] Gallery - Button (#18264)

d12c5dc

* WIP - add gallery button * Fix lint errors * Remove isString check from gallery button

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Gallery - Button #18264

[RNMobile] Gallery - Button #18264

mkevins commented Nov 4, 2019 •

edited

Loading

pinarol Nov 5, 2019

mkevins Nov 6, 2019

pinarol commented Nov 5, 2019

mkevins commented Nov 6, 2019

pinarol commented Nov 6, 2019

mkevins commented Nov 6, 2019 •

edited

Loading

mkevins commented Nov 6, 2019 •

edited

Loading

[RNMobile] Gallery - Button #18264

[RNMobile] Gallery - Button #18264

Conversation

mkevins commented Nov 4, 2019 • edited Loading

Description

To test

Checklist:

pinarol Nov 5, 2019

Choose a reason for hiding this comment

mkevins Nov 6, 2019

Choose a reason for hiding this comment

pinarol commented Nov 5, 2019

mkevins commented Nov 6, 2019

pinarol commented Nov 6, 2019

mkevins commented Nov 6, 2019 • edited Loading

mkevins commented Nov 6, 2019 • edited Loading

mkevins commented Nov 4, 2019 •

edited

Loading

mkevins commented Nov 6, 2019 •

edited

Loading

mkevins commented Nov 6, 2019 •

edited

Loading