Leaner code for the native mobile Toolbar #9064

hypest · 2018-08-17T01:32:43Z

Description

This PR should be considered as additional work on top of #9012.

Changes:

More code reuse by extracting into dedicated components what needs to be "nativized" and then "nativizing" those components/containers only
Nativized:
- Button
- Tooltip
- (SVG) Path
De-nativized:
- icon-button
- toolbar
Fix lint errors

Note:

Turns out there's we want to propagate a key prop down to the ToolbarButtonContainer component but key being a Special Prop for React it cannot be propagated. We need to mirror/replicate its value to another , differently named prop to pick it up inside the child component. See here.

How has this been tested?

Using this PR on the gutenberg mobile repo.

Types of changes

New components
Lint error fixes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

hypest · 2018-08-17T01:54:16Z

Some tests are failing. Example:

 FAIL  packages/components/src/dashicon/test/index.js
  Dashicon
    basic rendering
      ✓ should render an empty null element when icon is not provided (8ms)
      ✓ should render an empty null element when a non dashicon is provided (1ms)
      ✕ should render a SVG icon element when a matching icon is provided (9ms)
      ✓ should render an additional class to the SVG element (1ms)

  ● Dashicon › basic rendering › should render a SVG icon element when a matching icon is provided

    expect(received).toBe(expected) // Object.is equality

    Expected: "svg"
    Received: [Function _default]

    Difference:

      Comparing two different types of values. Expected string but received function.

      27 | 			expect( dashicon.hasClass( 'dashicon' ) ).toBe( true );
      28 | 			expect( dashicon.hasClass( 'dashicons-wordpress' ) ).toBe( true );
    > 29 | 			expect( dashicon.type() ).toBe( 'svg' );
         | 			                          ^
      30 | 			expect( dashicon.prop( 'xmlns' ) ).toBe( 'http://www.w3.org/2000/svg' );
      31 | 			expect( dashicon.prop( 'width' ) ).toBe( 20 );
      32 | 			expect( dashicon.prop( 'height' ) ).toBe( 20 );

      at Object.<anonymous> (packages/components/src/dashicon/test/index.js:29:30)

@gziolo or @youknowriad perhaps you have some hint about this one ^ ? Apparently it has something to do with the way the web version of the svg.js is defined here but, not sure how to fix it.

SergioEstevao · 2018-08-17T08:26:44Z

packages/components/src/dashicon/index.js

+ height={ size }
+ viewBox="0 0 20 20"
+ >
+ <Path d={ path } />


I'm not sure this will work on the web. If you see my original code on my PR for the web the Path element is defined using lowercase. i.e: path

I see what you did know you redefined Path also.

Hmm, I extracted the Path component to its own component and Path is imported here https://github.com/WordPress/gutenberg/pull/9064/files#diff-297ebddffcddddf088a365bab7f0909cR13. That should do it, although I think there's probably a problem with the way I've defined the web side variants (related to the comment).

I think this component is meant to be modified in the Dashicons repository first.

From the comments I saw on the file I thought this was the case, if we could a pointer to where to make the changes it will be great.

Dashicons are edited in https://github.com/WordPress/dashicons. Feel free to ping me for help, I can fasttrack any changes you'd like.

I think this might be the file to modify on the Dashicons repo: https://github.com/WordPress/dashicons/blob/master/sources/react/index-footer.jsx. Not sure yet how to integrate that repo/project and have it hot-reloading in the React Native app but yeah, let's see.

If you'd like to make a PR, I can approve it for you and ship it to Gutenberg.

@youknowriad , it would still be good to review the proposal in this PR and agree on whether we like/want it or not before migrating the change to the Dashicons repo, so we take advantage of the quick turnaround time we enjoy with the Gutenberg+ReactNative repo combo.

I wonder if the best approach here would be to have an alternative index.native.js for the whole dashicon component instead of extracting Path and Svg

SergioEstevao · 2018-08-17T08:43:06Z

packages/components/src/toolbar/index.js

 key={ [ indexOfSet, indexOfControl ].join() }
+ keyProp={ [ indexOfSet, indexOfControl ].join() }


Why do we repeat key and keyProp?

Ah, good catch, I should have mentioned this "gotcha" in the PR description. So, React has this set of "special" props that it doesn't propagate down to the child components. They call them Special Props. In this case, the key property cannot be picked up by the ToolbarButtonContainer component and we need to replicate it on purpose into a differently named prop, keyProp.

I'll update the PR description to include it for future ref.

SergioEstevao · 2018-08-17T08:57:14Z

packages/components/src/toolbar/toolbar-button-container.js

@@ -0,0 +1,8 @@
+export default ( props ) => (
+ <div
+ key={ props.keykeyProp }


shouldn't this be prop.keyProp only

My understanding of the docs is that key is used for the rendering reconciliation logic and since it was there in the original code (div) it needs to be here in the web variant of the component too. I can be wrong though as my React knowledge is limited still.

but keykeyPro?

Oh, I see now, that's a typo, good catch! Fixed with aed7db6.

SergioEstevao

Good work on this.

youknowriad · 2018-08-17T09:26:45Z

I feel this change has some impacts on the web, and since we've freezed developpement fo 3.6 for releasing, do you think it's possible to revert this change until the release?

youknowriad · 2018-08-17T09:43:04Z

Oh! I missed that this was merged against another branch, it's fine. Sorry for the confusion (The dashicons feedback is still valid though cc @jasmussen )

hypest · 2018-08-17T09:50:54Z

I know this PR is not merged to the parent #9012 but I'd like to add here that we should prolly name the functions/components in the modules so it's more clear which compos they are in the React inspector (notice the couple of "Unknown"s in the screenshot below):

hypest · 2018-08-17T10:00:37Z

And oh, btw, I manually tested the Gutenberg changes and the toolbar works fine, as in, nothing seems broken on the UI.

* Activate toolbar for RN. * Move toolbar to the top. * Implement RN icon buttons using SVG. * Add extra properties for toolbar selection. * Update url package entry point for RN. * Leaner code for the native mobile Toolbar (#9064) * Dedicated components for SVG, Path * Fix lint errors * Dedicated native components for button, icon-button, tooltip * Dedicated native mobile comps for Toolbar containers * Small code styling fix * Fix double "key" typo in prop name * Name the web side componets for better React debugging * Update the tests to cater for SVG, Path * Deconstruct arguments in single line. * Remove comment. * Remove unnecessary use of keyProp * Fix use of platform for iOS detection. * Update toolbar RN code to share more code with Web. * Update index.native.js * Address review comments. * Address eslint errors. * Remove extra white space. * Components: Add new primitives to work cross-platform * Keycodes: Include support for both iPad and iPhone when using external keyboard * Components: Use explicit name for Tooltip component in RN * Add token-list as a dependency to package.json to avoid random updates to the lock file * Return null to avoid failure on render. * Fix warning on SVG * Fix potential error on style. * Fix lint errors. * Fix lint error.

hypest added 4 commits August 17, 2018 02:54

Dedicated components for SVG, Path

12c6f5a

Fix lint errors

9b306ee

Dedicated native components for button, icon-button, tooltip

599774d

Dedicated native mobile comps for Toolbar containers

e961d17

hypest requested a review from SergioEstevao August 17, 2018 01:32

Small code styling fix

6f6f418

gziolo requested a review from a team August 17, 2018 06:54

gziolo added Framework Issues related to broader framework topics, especially as it relates to javascript Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Aug 17, 2018

gziolo requested review from aduth and youknowriad August 17, 2018 07:21

SergioEstevao reviewed Aug 17, 2018

View reviewed changes

Fix double "key" typo in prop name

aed7db6

SergioEstevao approved these changes Aug 17, 2018

View reviewed changes

SergioEstevao merged commit fefcb3b into rnmobile/toolbar Aug 17, 2018

SergioEstevao deleted the rnmobile/toolbar-extra-lean-code branch August 17, 2018 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leaner code for the native mobile Toolbar #9064

Leaner code for the native mobile Toolbar #9064

hypest commented Aug 17, 2018 •

edited

Loading

hypest commented Aug 17, 2018 •

edited

Loading

SergioEstevao Aug 17, 2018 •

edited

Loading

SergioEstevao Aug 17, 2018

hypest Aug 17, 2018

youknowriad Aug 17, 2018

SergioEstevao Aug 17, 2018

jasmussen Aug 17, 2018

hypest Aug 17, 2018

jasmussen Aug 17, 2018

hypest Aug 17, 2018

youknowriad Aug 17, 2018

SergioEstevao Aug 17, 2018

hypest Aug 17, 2018 •

edited

Loading

SergioEstevao Aug 17, 2018

hypest Aug 17, 2018

SergioEstevao Aug 17, 2018

hypest Aug 17, 2018

SergioEstevao left a comment

youknowriad commented Aug 17, 2018

youknowriad commented Aug 17, 2018

hypest commented Aug 17, 2018

hypest commented Aug 17, 2018

		key={ [ indexOfSet, indexOfControl ].join() }
		keyProp={ [ indexOfSet, indexOfControl ].join() }

Leaner code for the native mobile Toolbar #9064

Leaner code for the native mobile Toolbar #9064

Conversation

hypest commented Aug 17, 2018 • edited Loading

Description

Note:

How has this been tested?

Types of changes

Checklist:

hypest commented Aug 17, 2018 • edited Loading

SergioEstevao Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioEstevao left a comment

Choose a reason for hiding this comment

youknowriad commented Aug 17, 2018

youknowriad commented Aug 17, 2018

hypest commented Aug 17, 2018

hypest commented Aug 17, 2018

hypest commented Aug 17, 2018 •

edited

Loading

hypest commented Aug 17, 2018 •

edited

Loading

SergioEstevao Aug 17, 2018 •

edited

Loading

hypest Aug 17, 2018 •

edited

Loading