Skip to content
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.

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

RN: Add Icon Button Block Editor component. #25204

Merged
merged 8 commits into from
Sep 28, 2020
Merged

Conversation

enejb
Copy link
Contributor

@enejb enejb commented Sep 9, 2020

Description

This PR adds the menu-item component into its own component now called "IconButton".
The main reason for splitting up the component is to be able to reuse it in 3rd party blocks.

How has this been tested?

Since this a mobile only component for now it has been tested in the following way.

It has been tested using the mobile demo app on both Android and iOS.

  1. After clicking the (+) the insert should appear as before.
  2. Select the column block. Notice that the layout column selector looks the same as before.
  3. Select one of the layouts. Notice that it works as before.

Screenshots

Andorid
Screen Shot 2020-09-09 at 4 46 50 PM
Screen Shot 2020-09-09 at 4 46 32 PM

iOS
Screen Shot 2020-09-09 at 4 45 46 PM
Screen Shot 2020-09-09 at 4 45 01 PM

Types of changes

This is mostly a refactor so that we can use the same use the new component in 3rd party plugins.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@enejb enejb added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Package] Block editor /packages/block-editor labels Sep 9, 2020
@enejb enejb requested a review from geriux September 9, 2020 23:50
@enejb enejb self-assigned this Sep 9, 2020
@github-actions
Copy link

github-actions bot commented Sep 10, 2020

Size Change: -29.2 kB (2%)

Total Size: 1.17 MB

Filename Size Change
build/a11y/index.js 1.14 kB +1 B
build/api-fetch/index.js 3.35 kB +19 B (0%)
build/block-directory/index.js 8.53 kB +148 B (1%)
build/block-directory/style-rtl.css 943 B -10 B (1%)
build/block-directory/style.css 942 B -10 B (1%)
build/block-editor/index.js 128 kB +156 B (0%)
build/block-editor/style-rtl.css 11.1 kB +16 B (0%)
build/block-editor/style.css 11.1 kB +16 B (0%)
build/block-library/editor-rtl.css 8.57 kB -106 B (1%)
build/block-library/editor.css 8.58 kB -104 B (1%)
build/block-library/index.js 135 kB -3.27 kB (2%)
build/block-library/style-rtl.css 7.61 kB +15 B (0%)
build/block-library/style.css 7.6 kB +17 B (0%)
build/block-library/theme-rtl.css 741 B -13 B (1%)
build/block-library/theme.css 741 B -13 B (1%)
build/block-serialization-default-parser/index.js 1.78 kB +2 B (0%)
build/blocks/index.js 47.5 kB +115 B (0%)
build/components/index.js 167 kB -33.3 kB (19%) 👏
build/components/style-rtl.css 15.5 kB -45 B (0%)
build/components/style.css 15.5 kB -45 B (0%)
build/compose/index.js 9.42 kB -63 B (0%)
build/core-data/index.js 12 kB -169 B (1%)
build/data-controls/index.js 1.27 kB -6 B (0%)
build/data/index.js 8.43 kB +8 B (0%)
build/date/index.js 31.9 kB -1 B
build/dom/index.js 4.42 kB -20 B (0%)
build/edit-navigation/index.js 10.7 kB -752 B (7%)
build/edit-navigation/style-rtl.css 868 B -293 B (33%) 🎉
build/edit-navigation/style.css 871 B -292 B (33%) 🎉
build/edit-post/index.js 306 kB +73 B (0%)
build/edit-post/style-rtl.css 6.25 kB -9 B (0%)
build/edit-post/style.css 6.24 kB -9 B (0%)
build/edit-site/index.js 20.5 kB +979 B (4%)
build/edit-site/style-rtl.css 3.54 kB +483 B (13%) ⚠️
build/edit-site/style.css 3.54 kB +481 B (13%) ⚠️
build/edit-widgets/index.js 17.9 kB +5.99 kB (33%) 🚨
build/edit-widgets/style-rtl.css 2.82 kB +368 B (13%) ⚠️
build/edit-widgets/style.css 2.82 kB +369 B (13%) ⚠️
build/editor/index.js 45.4 kB +74 B (0%)
build/editor/style-rtl.css 3.83 kB +15 B (0%)
build/editor/style.css 3.82 kB +13 B (0%)
build/element/index.js 4.44 kB -8 B (0%)
build/format-library/index.js 7.49 kB -16 B (0%)
build/html-entities/index.js 621 B -1 B
build/i18n/index.js 3.55 kB +1 B
build/is-shallow-equal/index.js 709 B -2 B (0%)
build/list-reusable-blocks/index.js 3.02 kB +1 B
build/media-utils/index.js 5.12 kB +19 B (0%)
build/notices/index.js 1.69 kB -1 B
build/nux/index.js 3.27 kB +16 B (0%)
build/plugins/index.js 2.44 kB +5 B (0%)
build/redux-routine/index.js 2.85 kB +3 B (0%)
build/rich-text/index.js 13.7 kB -12 B (0%)
build/server-side-render/index.js 2.61 kB +5 B (0%)
build/url/index.js 4.06 kB -1 B
build/viewport/index.js 1.74 kB -1 B
build/warning/index.js 1.13 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/annotations/index.js 3.52 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 620 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Member

@geriux geriux left a comment

Choose a reason for hiding this comment

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

Hey @enejb 👋

Thanks for moving this component into its own so it can be reused!

I think this component (that is mobile-only for now) should be in the packages/components/src/mobile folder.

By moving it there, it'll prevent exposing the README file in the Block Editor Handbook docs.

What do you think?

@enejb
Copy link
Contributor Author

enejb commented Sep 10, 2020

Thanks for the feedback @geriux

I went and moved the component from block-editor/components to components/mobile.

It turns out that there is already a component. Called IconButton that got deprecated in favor of just Button.
I replaced the current IconButton from being included and replaced it with the new mobile/IconButton.

Do we need to worry about deprecation on mobile? I wasn't able to find any (deprecated usage of) IconButton in the code base.

Should we maybe rename the component to something else?

@geriux
Copy link
Member

geriux commented Sep 21, 2020

I went and moved the component from block-editor/components to components/mobile.

Thank you for the changes @enejb!

It turns out that there is already a component. Called IconButton that got deprecated in favor of just Button.
Do we need to worry about deprecation on mobile? I wasn't able to find any (deprecated usage of) IconButton in the code base.

I checked the PR when it was deprecated and looks like we were only using it in Media & Text block but it was changed in favor of the new Button so I think we shouldn't worry about the deprecation.

Should we maybe rename the component to something else?

I think we should, it might cause confusion if other people start seeing references to it, what I don't know is what name could work for it =P

@enejb
Copy link
Contributor Author

enejb commented Sep 22, 2020

@geriux I updated the component to be called InserterButton. I don't have a better name yet :(

What do you think? Would that work?

Copy link
Member

@geriux geriux left a comment

Choose a reason for hiding this comment

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

Hey @enejb just left two comments. I like the InsertButton name by the way 👍

@@ -16,6 +16,11 @@ import { __ } from '@wordpress/i18n';
*/
import styles from './style.scss';

export const inserterButtonStyles = {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think if instead of exporting the styles separately we do something like we do for the BottomSheet ?

So we have something like:

const InserterButton = withPreferredColorScheme( MenuItem );

InserterButton.Styles = inserterButtonStyles;

export default InserterButton;

Copy link
Contributor Author

@enejb enejb Sep 24, 2020

Choose a reason for hiding this comment

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

Yeah I like that pattern. Lets go with that, thanks for pointing it out.

packages/components/src/mobile/inserter-button/README.md Outdated Show resolved Hide resolved
Copy link
Member

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the changes @enejb!

@enejb enejb merged commit b9ff7ea into master Sep 28, 2020
@enejb enejb deleted the add/mobile-inserter-menu branch September 28, 2020 22:41
@github-actions github-actions bot added this to the Gutenberg 9.1 milestone Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Package] Block editor /packages/block-editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants