Skip to content

[RNMobile] iOS DarkMode toolbar buttons #17356

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.

Sign up for GitHub

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

Merged
etoledom merged 6 commits into from
Oct 3, 2019
Merged

[RNMobile] iOS DarkMode toolbar buttons #17356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3823c6c
Send theme and active props to button children
etoledom Sep 6, 2019
fec60d5
Remove ariaPressed prop from icon
etoledom Sep 6, 2019
d9d6d9e
DarkMode color improvement on toolbar separator
etoledom Sep 6, 2019
e03c6f7
Updated using new DarkMode refactor
etoledom Sep 26, 2019
640b17f
Improved names. Avoiding mentioning "theme" to refer to color schemes.
etoledom Sep 26, 2019
f4e5629
Merge branch 'master' into rnmobile/darkmode-toolbar-buttons
etoledom Sep 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Improved names. Avoiding mentioning "theme" to refer to color schemes.
  • Loading branch information
@etoledom
etoledom committed Sep 27, 2019
commit 640b17ffa55b6d00b57203127fe269a2a6d79b12
2 changes: 1 addition & 1 deletion packages/components/src/button/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export function Button( props ) {
const subscriptInactive = getStylesFromColorScheme( styles.subscriptInactive, styles.subscriptInactiveDark );

const newChildren = Children.map( compact( children ), ( child ) => {
Copy link
Contributor

@Tug Tug Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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>

Copy link
Contributor Author

@etoledom etoledom Sep 27, 2019

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

@Tug Tug Sep 27, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@etoledom etoledom Sep 27, 2019

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

@mchowning mchowning Oct 2, 2019

Choose a reason for hiding this comment

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

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

return cloneElement( child, { theme: props.theme, active: ariaPressed } );
return cloneElement( child, { colorScheme: props.preferredColorScheme, active: ariaPressed } );
} );

return (
Expand Down
6 changes: 3 additions & 3 deletions packages/components/src/primitives/svg/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ export {
} from 'react-native-svg';

export const SVG = ( props ) => {
const theme = props.theme || 'light';
const colorScheme = props.colorScheme || 'light';
const stylesFromClasses = ( props.className || '' ).split( ' ' ).map( ( element ) => styles[ element ] ).filter( Boolean );
const stylesFromAriaPressed = props.active ? styles[ 'is-active' ] : styles[ 'components-toolbar__control-' + theme ];
const styleValues = Object.assign( {}, props.style, stylesFromAriaPressed, ...stylesFromClasses );
const defaultStyle = props.active ? styles[ 'is-active' ] : styles[ 'components-toolbar__control-' + colorScheme ];
const styleValues = Object.assign( {}, props.style, defaultStyle, ...stylesFromClasses );

const safeProps = { ...props, style: styleValues };

Expand Down