[RNMobile] iOS DarkMode toolbar buttons #17356

etoledom · 2019-09-06T07:03:11Z

gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1357

Description

This PR allow us to add different styles to DashIcons inside IconButton component depending on the the style theme (Dark/Light mode).

It also enhance the DarkMode theme for Toolbar buttons (and the toolbar separator)

With this change, is not needed to send ariaPressed prop from IconButton. This is a change that was requested long ago, (even though we still are sending it button states).

A second idea is to send styles directly to DashIcon from the Button, to avoid having to declare styles for every new class. I'm not sure if this is something we want to change though.

As always any moment is welcome!

To test:

Run the example app with Xcode 11 beta
Check that the toolbar colors are correct on the toolbar (following the screenshots)
Check that the rest of the icons in the app shows properly
- In particular the BottomSheet icons (Add link / Image settings)
Check that the icon Subscript is colored properly (Numbers on H1, H2, etc...)

packages/components/src/primitives/svg/index.native.js

etoledom · 2019-09-26T11:49:21Z

Updated this PR implementing the DarkMode refactor from: #17552

@Tug or @mchowning - Could any of you please take a look? Thank you!

gziolo · 2019-09-26T12:25:46Z

packages/components/src/icon-button/index.js

@@ -56,7 +55,7 @@ function IconButton( props, ref ) {
 			className={ classes }
 			ref={ ref }
 		>
-			<Icon icon={ icon } ariaPressed={ ariaPressed } />
+			<Icon icon={ icon } />


Are you sure that it's safe to stop passing down this prop?

Yes, it's basically being replaced by this line here:
https://github.com/WordPress/gutenberg/blob/82a96dd640354ba5068dee5a10e6195e96ea65bb/packages/components/src/button/index.native.js#L97
Were we send the colorScheme and the active state to all children of Button.

But considering @Tug's comment here: https://github.com/WordPress/gutenberg/pull/17356/files#r328582023 I might need to change this anyway.

What do you think?

I checked the history and it looks like ariaPressed was introduced in #11827. It was never really used in the web code. Should it be also removed in other places?

In particular, I think about Dashicon implementation which used to pass this prop down but it seems to longer be necessary. See:

gutenberg/packages/components/src/dashicon/index.js

Line 895 in eb3a82f

const iconClass = getIconClassName( icon, className, ariaPressed );

Filed here: #17741.

Tug

I left a comment about the way we pass the colorScheme props down tho children.

An alternative would be to consider having the colorScheme available in a more global context. Basically have DarkModeProvider in the EditorProvider (we could rename mode to colorScheme), we could get rid of all the HOCs but we would need to use <DarkModeContext.Consumer> ( { colorScheme, active } ) ) => whenever we want to access that value

Tug · 2019-09-26T12:21:27Z

packages/components/src/button/index.native.js

@@ -80,6 +91,12 @@ export default function Button( props ) {
 		states.push( 'disabled' );
 	}

+	const subscriptInactive = getStylesFromColorScheme( styles.subscriptInactive, styles.subscriptInactiveDark );
+
+	const newChildren = Children.map( compact( children ), ( child ) => {


I'd rather we use a context for this:

// in Button: export const ButtonContext = React.createContext( { active: false, colorScheme: 'light', } ); export function Button( props ) { ... const buttonContext = { active: props.active, colorScheme: props.colorScheme, }; { Children.count( children ) && <ButtonContext.Provider value={ buttonContext }> { children } </ButtonContext.Provider> } ... // in IconButton <ButtonContext.Consumer> { ( { colorScheme, active } ) => <Icon icon={ icon } ariaPressed={ active } colorScheme={ colorScheme } /> </ButtonContext.Consumer>

I copied the idea from here: https://github.com/WordPress/gutenberg/blob/82a96dd640354ba5068dee5a10e6195e96ea65bb/packages/components/src/primitives/block-quotation/index.native.js#L15

And the main idea was to clear up IconButton from sending the ariaPressed or active related states.

I like that we can get enough information to style any child of button, but you are right that this might not be the best option.

I'll give this a try!

Thinking about it, I guess using cloneElement should not have any impact on performance, it's just a bit more limited in terms of features. Basically, you'll only be able to access those props from the direct children of the button, whereas a consumer could be added anywhere. If direct children is fine then I guess we can keep it that way.

Yes, the main (and only, so far) usage of this is the IconButton where the Icon component is a direct child.
So, if you think it's fine to keep this, let's move on 👍

I restarted a CI job to get the ✅ from travis.
Thanks for your review!

I didn't know about React Context before. Thanks for leaving this comment @Tug !

etoledom · 2019-09-27T10:25:00Z

@Tug I'm thinking to wait until #17228 is merged before merging this one. What do you think?

This helps to properly style buton icons for the given theme. And removes the need to pass aria-pressed to icons directly.

etoledom · 2019-09-30T16:34:56Z

@Tug if you agree with this implementation, could you please ✅ to move forward? :)
Thank you!

Tug

Code LGTM 👍

etoledom · 2019-10-02T09:26:00Z

Thank you @Tug !
@mchowning could you please give this PR a run to be sure it's working as expected?

Thank you!

mchowning

Tested and everything looks good. Nice job @etoledom !

etoledom · 2019-10-03T06:16:06Z

Thank you all!

gziolo · 2019-10-03T07:43:46Z

It should fix #17695, thank you for working on it 👍

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 Sep 6, 2019

etoledom requested review from ajitbohra, chrisvanpatten, gziolo, jorgefilipecosta, talldan and youknowriad as code owners September 6, 2019 07:03

etoledom self-assigned this Sep 6, 2019

etoledom changed the title ~~Rnmobile/darkmode toolbar buttons~~ [RNMobile] iOS DarkMode toolbar buttons Sep 6, 2019

etoledom mentioned this pull request Sep 6, 2019

iOS DarkMode toolbar buttons wordpress-mobile/gutenberg-mobile#1357

Merged

etoledom requested a review from Tug September 6, 2019 07:08

gziolo reviewed Sep 9, 2019

View reviewed changes

packages/components/src/primitives/svg/index.native.js Outdated Show resolved Hide resolved

etoledom requested review from gziolo and mchowning September 26, 2019 11:47

gziolo reviewed Sep 26, 2019

View reviewed changes

Tug reviewed Sep 26, 2019

View reviewed changes

etoledom requested a review from Tug September 27, 2019 10:21

etoledom changed the base branch from rnmobile/master to master September 27, 2019 16:05

etoledom changed the base branch from master to rnmobile/master September 27, 2019 16:08

etoledom added 5 commits September 27, 2019 18:15

Send theme and active props to button children

3823c6c

This helps to properly style buton icons for the given theme. And removes the need to pass aria-pressed to icons directly.

Remove ariaPressed prop from icon

fec60d5

DarkMode color improvement on toolbar separator

d9d6d9e

Updated using new DarkMode refactor

e03c6f7

Improved names. Avoiding mentioning "theme" to refer to color schemes.

640b17f

etoledom force-pushed the rnmobile/darkmode-toolbar-buttons branch from 82a96dd to 640b17f Compare September 27, 2019 16:32

etoledom requested review from adamsilverstein, aduth and atimmer as code owners September 27, 2019 16:32

etoledom removed request for SergioEstevao, talldan, Soean, swissspidy, ntwb, ajitbohra, chrisvanpatten, mmtr, nerrad, aduth, adamsilverstein, TimothyBJacobs, ellatrix, dmsnell and jorgefilipecosta September 27, 2019 16:33

Merge branch 'master' into rnmobile/darkmode-toolbar-buttons

f4e5629

etoledom mentioned this pull request Sep 30, 2019

[RNMobile] Fix crash bringing gutenberg master to mobile #17228

Merged

Tug approved these changes Oct 2, 2019

View reviewed changes

desaiuditd mentioned this pull request Oct 2, 2019

Fix the browser warning for IconButton component. #17695

Closed

5 tasks

mchowning approved these changes Oct 2, 2019

View reviewed changes

etoledom merged commit 5c5dc12 into master Oct 3, 2019

etoledom deleted the rnmobile/darkmode-toolbar-buttons branch October 3, 2019 06:15

gziolo mentioned this pull request Oct 3, 2019

Components: Cleanup Dashicons component #17741

Merged

etoledom mentioned this pull request Oct 7, 2019

[RNMobile] Native mobile release v1.14.0 #17676

Merged

4 tasks

youknowriad added this to the Gutenberg 6.7 milestone Oct 14, 2019

etoledom mentioned this pull request Oct 15, 2019

Gutenberg: Update to v1.15.0 wordpress-mobile/WordPress-iOS#12691

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] iOS DarkMode toolbar buttons #17356

[RNMobile] iOS DarkMode toolbar buttons #17356

etoledom commented Sep 6, 2019 •

edited

Loading

etoledom commented Sep 26, 2019

gziolo Sep 26, 2019

etoledom Sep 27, 2019

gziolo Oct 3, 2019 •

edited

Loading

gziolo Oct 3, 2019

Tug left a comment

Tug Sep 26, 2019 •

edited

Loading

etoledom Sep 27, 2019

Tug Sep 27, 2019

etoledom Sep 27, 2019

mchowning Oct 2, 2019

etoledom commented Sep 27, 2019

etoledom commented Sep 30, 2019

Tug left a comment

etoledom commented Oct 2, 2019

mchowning left a comment

etoledom commented Oct 3, 2019

gziolo commented Oct 3, 2019

[RNMobile] iOS DarkMode toolbar buttons #17356

[RNMobile] iOS DarkMode toolbar buttons #17356

Conversation

etoledom commented Sep 6, 2019 • edited Loading

Description

etoledom commented Sep 26, 2019

gziolo Sep 26, 2019

Choose a reason for hiding this comment

etoledom Sep 27, 2019

Choose a reason for hiding this comment

gziolo Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

gziolo Oct 3, 2019

Choose a reason for hiding this comment

Tug left a comment

Choose a reason for hiding this comment

Tug Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

etoledom Sep 27, 2019

Choose a reason for hiding this comment

Tug Sep 27, 2019

Choose a reason for hiding this comment

etoledom Sep 27, 2019

Choose a reason for hiding this comment

mchowning Oct 2, 2019

Choose a reason for hiding this comment

etoledom commented Sep 27, 2019

etoledom commented Sep 30, 2019

Tug left a comment

Choose a reason for hiding this comment

etoledom commented Oct 2, 2019

mchowning left a comment

Choose a reason for hiding this comment

etoledom commented Oct 3, 2019

gziolo commented Oct 3, 2019

etoledom commented Sep 6, 2019 •

edited

Loading

gziolo Oct 3, 2019 •

edited

Loading

Tug Sep 26, 2019 •

edited

Loading