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.

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

Jump to bottom

Refactor Buttons block native edit component to use hooks. #25636

Merged
merged 2 commits into from
Oct 13, 2020
Merged

Refactor Buttons block native edit component to use hooks. #25636

merged 2 commits into from
Oct 13, 2020

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Sep 25, 2020 •
edited
Loading

Description

This PR refactors the Buttons block native edit implementation to use useSelect and useDispatch instead of their HOC counterparts.

How has this been tested?

It hasn't. I need someone from the native mobile team to test out this PR and make sure everything works.

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.

@ZebulanStanphill ZebulanStanphill added [Type] Code Quality Issues or PRs that relate to code quality [Package] Block library /packages/block-library [Block] Buttons Affects the Buttons Block labels Sep 25, 2020
@github-actions
Copy link

github-actions bot commented Sep 25, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 1.19 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.52 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 667 B 0 B
build/block-directory/index.js 8.56 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 130 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 8.65 kB 0 B
build/block-library/editor.css 8.65 kB 0 B
build/block-library/index.js 145 kB 0 B
build/block-library/style-rtl.css 7.7 kB 0 B
build/block-library/style.css 7.7 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.6 kB 0 B
build/components/index.js 169 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.63 kB 0 B
build/core-data/index.js 12 kB 0 B
build/data-controls/index.js 685 B 0 B
build/data/index.js 8.63 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/index.js 10.6 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.29 kB 0 B
build/edit-post/style.css 6.28 kB 0 B
build/edit-site/index.js 21.3 kB 0 B
build/edit-site/style-rtl.css 3.8 kB 0 B
build/edit-site/style.css 3.81 kB 0 B
build/edit-widgets/index.js 21.2 kB 0 B
build/edit-widgets/style-rtl.css 3.03 kB 0 B
build/edit-widgets/style.css 3.03 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 45.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.49 kB 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/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 710 B 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/index.js 3.02 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/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.6 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.75 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@geriux geriux 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 25, 2020
@Tug Tug requested review from lukewalczak, Tug and hypest and removed request for antonis October 12, 2020 10:08
Tug
Tug approved these changes Oct 12, 2020
View reviewed changes
Copy link
Contributor

@Tug Tug left a comment

Choose a reason for hiding this comment

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

Change looks good and works well.

I had a few suggestions for changes that would improve the code slightly, would appreciate if you address those before merging.

packages/block-library/src/buttons/edit.native.js Show resolved Hide resolved
packages/block-library/src/buttons/edit.native.js Outdated
@@ -46,8 +70,27 @@ function ButtonsEdit( {
}
}, [ sizes ] );

function onAddNextButton( selectedId ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor, but using const onAddNextButton = ( selectedId ) => { here instead would be slightly better as onAddNextButton would be defined when this line is evaluated. function declaration are ok but they're hoisted to the top of this react component so that might lead to inconsistencies.
Same for onDelete

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally, I prefer function declarations for non-callback functions, but I noticed that this function was only used as an argument for debounceAddNextButton, so I decided to just have an onAddNextButton variable set to the result of debounce, with the function passed in as a callback.

I decided onDelete didn't add much value (and might have actually just been complicating things by making your eyes jump back up the code just for a single-line function), so I just moved that function inline to where it was used.

packages/block-library/src/buttons/edit.native.js Outdated Show resolved Hide resolved
@ZebulanStanphill ZebulanStanphill force-pushed the update/polish-buttons-edit-native branch from 44c2241 to a6bdda0 Compare October 13, 2020 02:44
@lukewalczak
Copy link
Member

lukewalczak commented Oct 13, 2020 •
edited
Loading

Code changes looks properly, and do not expect any regression, however I will test it going through Button sanity test cases:

  • - Button 1 (whole group)
  • - Button 2 (whole group)
  • - Button 3 (whole group)
  • - Button 4 (whole group)
  • - Button 5 (whole group)
  • - Button 6 (whole group)
  • - Button 7 (whole group)

Note: To test it properly I have to locally merge changes from master since was not able to type any character inside the Button

@ZebulanStanphill ZebulanStanphill merged commit a9267fe into master Oct 13, 2020
@ZebulanStanphill ZebulanStanphill deleted the update/polish-buttons-edit-native branch October 13, 2020 14:30
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 13, 2020
@geriux
Copy link
Member

geriux commented Oct 16, 2020

Code changes looks properly, and do not expect any regression, however I will test it going through Button sanity test cases:

Hey @lukewalczak 👋

I was working on a Buttons block PR and saw a possible regression from this PR but I want to double check with you. Looks like now the appender to add new buttons (when a Button is selected) is no longer showing. Is this expected?

Before Current

@lukewalczak
Copy link
Member

Hey @geriux, are you sure that PR is a root of regression (I wouldn't consider it as a intentional behavior)? During testing I didn't notice it 🤔

@lukewalczak
Copy link
Member

@geriux probably, you're right and I had to overlooked that. Should I investigate that or you're on it currently?

@geriux
Copy link
Member

geriux commented Oct 16, 2020

@lukewalczak fixed the issue mentioned above in this PR.

@hypest hypest removed their request for review October 16, 2020 13:09
@ceyhun ceyhun mentioned this pull request Oct 16, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukewalczak lukewalczak lukewalczak approved these changes

@Tug Tug Tug approved these changes

Assignees
No one assigned
Labels
[Block] Buttons Affects the Buttons Block 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 library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 9.2
Development

Successfully merging this pull request may close these issues.
4 participants
@ZebulanStanphill @lukewalczak @geriux @Tug