Rnmobile/native toolbar component ui by marecar3 · Pull Request #11827 · WordPress/gutenberg

marecar3 · 2018-11-13T22:13:42Z

Description

This PR resolves part of the Toolbar issue. It implements Toolbar and Toolbar Button style by the specs.

What's done :

Implement Toolbar Style

Implement toolbar button style by specs
Implement toolbar style by specs

To test:

Please checkout the gutenberg-mobile PR project and follow the instructions.

Implement toolbar button style

koke · 2018-11-14T08:39:18Z

I'll let @iamthomasbishop comment on this since I don't have the exact right answers, but things I notice so far:

Color seems a darker more neutral gray than the blue-ish in the mockups
Borders at the top and separating groups of buttons seem darker and thicker
I don't see any selected states in the mockups, but I'd expect some different accent color, not a border for the button

It does look much nicer than before though 😁

gziolo · 2018-11-14T09:23:21Z

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

 */
 import { TouchableOpacity, Text, View } from 'react-native';

+import styles from './button-style.scss';


Noting that it gives a false impression that this style file could be used by web because it uses .scss extension without any indication that it can be used only by mobile. Please also note that we have a different convention for web. We use one file per folder and follow these guidelines when creating class names:
https://github.com/WordPress/gutenberg/blob/master/docs/reference/coding-guidelines.md#css

See also how files are named and included:
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/style.scss

It feels like this is something you should standardize somehow sooner than later.

If you want to continue to have it working the way it exists as of today, I would be fine if you rename those files to style.native.scss.

Hey @gziolo , I have updated css files according your instructions.

Thanks, looks good 👍

Change insert icon color to wp blue

koke · 2018-11-20T10:57:43Z

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

 */
 import { TouchableOpacity, Text, View } from 'react-native';

+import styles from './style.native.scss';


Since we're not sharing styles with the web, I wonder if using StyleSheet.create would look cleaner here to deal with conditional styling.

It does make sense. Fixed.

koke · 2018-11-20T11:12:45Z

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

 		let element = (
 			<Button aria-label={ label } { ...additionalProps } className={ classes }>
-				{ isString( icon ) ? <Dashicon icon={ icon } /> : icon }
+				{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ariaPressed}/> : icon }


aria-pressed is specific to the web. Instead of trying to hijack that, I'd prefer if we rename it to something more neutral (pressed?), then make sure each platform does what it needs to render correctly.

I have that on a mind, but it seems that we are using aria-pressed in button/index.native.js so I wanted to be consistent with other parts of the code.

koke · 2018-11-20T11:13:42Z

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

 	);
 };
+
+export const Path = ( props ) => {


I don't understand why we need this wrapper, can you explain more?

Good catch! This was a valid solution at some point, but after a few iterations, it lost purpose!
Fixed.

Remove scss file and use proper StyleSheet to make code cleaner

marecar3 · 2018-11-21T10:11:13Z

Hey @iamthomasbishop

Definitely looking closer!

I'm wondering if the icons are starting look too dim now that we have a stronger selected style. I'm going to confirm what value we're using in Aztec to make sure we're aligning and providing appropriate contrast, and will adjust if necessary.

It would be good to have it today as we want to merge this PR.

The numbers on the Heading icons feel odd because of the way we're appending the number programmatically to the icons. If we can weight those numbers to be bold and bring them down to the baseline of the icon, maybe that will fix the issue. For reference, here's what they're doing in Core:

My original designs had partial dividers between the heading buttons and inline tools (bold/italic/strike/link). Do we have the capabilities to do this?

Nope. It's not easy at this point to do that.

Updated design :

koke · 2018-11-21T10:41:23Z

EDIT: I hadn't pulled the latest changes correctly

~~I just tested this on Android and it seems it's not picking the right colors when "pressed"~~

koke · 2018-11-21T14:04:00Z

This is looking great. Some minor details:

A marginLeft of -3 looks better on both platforms, not just Android.
On iOS the numbers are still slightly up (iPhone X), but I'm worried that getting to specific with fractional pixels there might work in some phones but not others. I think having separate SVGs will be a better approach for this.
When the button is pressed (while the finger is tapping it), I think the iOS one looks good, but Android is showing different colors:

iamthomasbishop · 2018-11-21T16:23:37Z

A little update on the default/un-selected button icon colors, as discussed in dm w/ @marecar3:

I did a little digging to figure out why our icons looked too dim compared to Aztec, and I realized that we were using a custom shade for the toolbar in Aztec, which is slightly darker than $gray (I should've know, I was the one that worked on this 🤦‍♂️ ). That color is: #7B9AB1 (in between our standard $gray and $gray-darken-10). This should add some needed contrast :)

# Conflicts: # packages/components/src/icon-button/index.js

marecar3 · 2018-11-22T10:11:15Z

@iamthomasbishop

A little update on the default/un-selected button icon colors, as discussed in dm w/ @marecar3:
I did a little digging to figure out why our icons looked too dim compared to Aztec, and I realized that we were using a custom shade for the toolbar in Aztec, which is slightly darker than $gray (I should've know, I was the one that worked on this 🤦‍♂️ ). That color is: #7B9AB1 (in between our standard $gray and $gray-darken-10). This should add some needed contrast :)

koke

Looking great 👏

youknowriad · 2018-11-29T11:44:00Z

packages/components/src/dashicon/index.js

 		}

-		const iconClass = [ 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' );
+		const iconClass = IconClass( this.props );


Is this a component or or a function, if it's a function it should be lower-case.
Also, we shouldn't touch this file directly, it's synced from the Dashicon repository I think, which means these changes can get removed inadvertently.

youknowriad · 2018-11-29T11:45:17Z

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

 		let element = (
 			<Button aria-label={ label } { ...additionalProps } className={ classes }>
-				{ isString( icon ) ? <Dashicon icon={ icon } /> : icon }
+				{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } /> : icon }


Not sure I understand this change? why do we need aria-pressed here since we're applying it to the parent button?

The underlying issue is that we need to find a way to set the color of a Dashicon. Since the web has CSS it can use class names, inheritance, and fill: currentColor to get to that. But we don't have that on native.

Ideally we'd be able to set color or style on a Dashicon, I took an alternate approach in #12403, but that was easier since I was using Dashicon directly. For this we'd have to make everything in the hierarchy forward the style down to Dashicon. As mentioned on that PR, you can set extraProps in IconButton, but those will go to the Button, not the icon.

We just found something that worked to keep us moving although we were aware it wasn't great, so I'm happy to discuss a better solution 😁

I'm concerned about these small tweaks having an impact on the web code base, can't we create an icon-button native version instead?

The underlying issue is that we need to find a way to set the color of a Dashicon. Since the web has CSS it can use class names, inheritance, and fill: currentColor to get to that. But we don't have that on native.

@koke Can you point me to where this occurs? As best I can follow, the main impact of the ariaPressed prop is in changing the class behavior of Dashicon to apply dashicon-active in the native context, but I see no reference to dashicon-active anywhere in Gutenberg or Gutenberg Mobile aside from where we apply it.

I'd tend to agree with @youknowriad here. "Pressed" does not resound to me as a characteristic of an icon, and thus it does not make sense as a prop. It does, however, seem natural to describe the state of an IconButton.

I think this is what's setting the style https://github.com/WordPress/gutenberg/blob/master/packages/components/src/primitives/svg/style.native.scss#L6

I agree that this isn't ideal. It seems like the introduction of iconProps in #13977 could be a good solution to pass custom styles from IconButton to Dashicon?

I don't know that #13977 is an improvement here, as I expect you'd continue to treat it as a pressed icon, which is the root of the issue ("pressed" is not a sensible state of an icon).

Ah, that's right.
It looks like the right logic here would be that IconButton set different style props on Dashicon depending on isActive. This seems to be what the web is doing, although via CSS.
However this is overriden by CSS in the ToolbarButton component.

We keep bumping into this on native: the web can rely on nested CSS definition to pass style down to lower level components, but we can't do that unless we pass them explicitly. I still think ToolbarButton should be somehow deciding what the style for Dashicon is when normal/pressed, which is why now having iconProps in IconButton seems like a way out.

@etoledom can you handle this (removing ariaPressed from Dashicon) as part of #13977?

marecar3 added 7 commits November 8, 2018 09:48

Change dashicon color

ff3cd91

Implement toolbar button style

Merge branch 'master' into rnmobile/native-toolbar-component-ui

e28480f

Fixed Toolbar style

3cff075

Merge branch 'master' into rnmobile/native-toolbar-component-ui

6a06306

Style refactor

442a6eb

Small refinment

5c6ae4f

Merge branch 'master' into rnmobile/native-toolbar-component-ui

b60acce

marecar3 requested review from gziolo, hypest and koke November 13, 2018 22:13

marecar3 added 2 commits November 13, 2018 23:43

Small fixes

565bd08

Small fixes

125a576

marecar3 mentioned this pull request Nov 13, 2018

Feature/toolbar component ui wordpress-mobile/gutenberg-mobile#235

Merged

2 tasks

Small fix

7588291

gziolo reviewed Nov 14, 2018

View reviewed changes

gziolo 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 Nov 14, 2018

marecar3 added 8 commits November 19, 2018 11:16

Temp push

826bec4

Implemented selected state

c79a483

Change insert icon color to wp blue

Fixed naming

a5aa2f2

Small fixes

7c6c0f4

Rename css files

4aaac80

Lint fixes

54440b6

Added EOF line

ee7a909

Merge branch 'master' into rnmobile/native-toolbar-component-ui

2560918

koke reviewed Nov 20, 2018

View reviewed changes

marecar3 added 4 commits November 20, 2018 14:13

Removed path const

2fa504d

Remove scss file and use proper StyleSheet to make code cleaner

Merge branch 'master' into rnmobile/native-toolbar-component-ui

66e210f

Merge branch 'master' into rnmobile/native-toolbar-component-ui

45643fa

Small fix

4fed682

marecar3 added 2 commits November 21, 2018 09:56

Flow error fix

51956f8

Fixes from comments

746c90a

marecar3 added 2 commits November 21, 2018 11:37

FIxed lint errors

49e82b4

Merge branch 'master' into rnmobile/native-toolbar-component-ui

b22ad2e

Fixes from comments

56640cd

marecar3 added 2 commits November 22, 2018 11:08

Updated dashicon unselected state color

591ce94

Merge branch 'master' into rnmobile/native-toolbar-component-ui

0c81045

# Conflicts: # packages/components/src/icon-button/index.js

marecar3 added 3 commits November 22, 2018 13:30

Fixes from comments

5e6e9d2

Fixed lint errors

bfdab84

Merge branch 'master' into rnmobile/native-toolbar-component-ui

a9aa8e5

koke approved these changes Nov 22, 2018

View reviewed changes

Merge branch 'master' into rnmobile/native-toolbar-component-ui

168f08b

marecar3 merged commit 017f31b into master Nov 22, 2018

koke mentioned this pull request Nov 28, 2018

Allow custom style in dashicon/svg #12403

Closed

youknowriad reviewed Nov 29, 2018

View reviewed changes

youknowriad added this to the 4.7 milestone Nov 29, 2018

youknowriad deleted the rnmobile/native-toolbar-component-ui branch November 29, 2018 11:46

mtias modified the milestones: 4.7, Mobile Alpha Dec 6, 2018

This was referenced Mar 1, 2019

Mobile: Dashicon color override #13977

Closed

Make IconButton able to be referenced. #14163

Merged

This was referenced Oct 3, 2019

[RNMobile] iOS DarkMode toolbar buttons #17356

Merged

Components: Cleanup Dashicons component #17741

Merged

Conversation

marecar3 commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What's done :

Uh oh!

koke commented Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marecar3 commented Nov 21, 2018

Uh oh!

koke commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koke commented Nov 21, 2018

Uh oh!

iamthomasbishop commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marecar3 commented Nov 22, 2018

Uh oh!

koke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

marecar3 commented Nov 13, 2018 •

edited

Loading

koke commented Nov 14, 2018 •

edited

Loading

koke commented Nov 21, 2018 •

edited

Loading

iamthomasbishop commented Nov 21, 2018 •

edited

Loading

aduth Mar 1, 2019 •

edited

Loading