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 more block #23758

Merged
merged 5 commits into from
Jul 9, 2020
Merged

Refactor more block #23758

merged 5 commits into from
Jul 9, 2020

Conversation

ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Jul 7, 2020 •
edited
Loading

Description

This PR refactors More block to a function component.

Related #22890

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.

@ntsekouras ntsekouras self-assigned this Jul 7, 2020
@ntsekouras ntsekouras added [Block] More Affects the More Block - used for displaying the 'Read More' link [Type] Code Quality Issues or PRs that relate to code quality labels Jul 7, 2020
@github-actions
Copy link

github-actions bot commented Jul 7, 2020 •
edited
Loading

Size Change: -90 B (0%)

Total Size: 1.14 MB

Filename Size Change
build/block-editor/index.js 115 kB -3 B (0%)
build/block-library/index.js 130 kB -91 B (0%)
build/components/index.js 199 kB +3 B (0%)
build/edit-post/index.js 304 kB +1 B
build/edit-site/index.js 16.6 kB +1 B
build/edit-widgets/index.js 9.35 kB +2 B (0%)
build/editor/index.js 45 kB +1 B
build/list-reusable-blocks/index.js 3.12 kB -1 B
build/media-utils/index.js 5.32 kB -1 B
build/plugins/index.js 2.56 kB -1 B
build/token-list/index.js 1.28 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.67 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.54 kB 0 B
build/block-library/editor.css 7.54 kB 0 B
build/block-library/style-rtl.css 7.75 kB 0 B
build/block-library/style.css 7.76 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.56 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.46 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/style-rtl.css 5.59 kB 0 B
build/edit-post/style.css 5.58 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 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 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 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/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.4 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ZebulanStanphill
Copy link
Member

There were some unit tests that need updating. I have pushed a commit updating them.

@ntsekouras
Copy link
Contributor Author

Wow @ZebulanStanphill , that was fast :) ! Thanks!!

I left it in draft, as I am out for today in a minute and wanted to make an issue first, that this PR fixes too ( about empty saved value ).

ZebulanStanphill
ZebulanStanphill reviewed Jul 7, 2020
View reviewed changes
packages/block-library/src/more/edit.js Outdated
};
}
export default function MoreEdit( {
// we provide a default value as previously we allowed to save `undefined`, even though it wasn't clear to the UI
Copy link
Member

Choose a reason for hiding this comment

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

This comment isn't really clear to me. Also, most comments should use sentence capitalization and punctuation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, most comments should use sentence capitalization and punctuation.

I'll keep it in mind. Thanks! -- I changed the comment.

@ntsekouras ntsekouras marked this pull request as ready for review July 8, 2020 07:23
@ntsekouras ntsekouras requested review from gziolo and mcsf July 8, 2020 07:28
@mcsf
Copy link
Contributor

mcsf commented Jul 8, 2020

doesn't let the user leave an empty text that leads to misleading results.

Hm, this is a tricky situation. The thing is that this fix is a breaking change for WP's old more feature. By automatically adopting DEFAULT_TEXT as the text attribute for this block, we lose the ability to let the PHP environment (e.g. theme, hooks, or any other caller of get_the_content) decide what the $more_link_text should be when no value is present inside the <!--more tag.

I would also point out that, in general, we should be sceptical anytime we see { attrName = DEFAULT } = attributes in a block's Edit component, because this bypasses Gutenberg's native understanding of attributes and their default values. In other words, we should always prefer:

"attributes": {
	"attrName": {
		"type": "string",
		"default": "xyz"
	},

The More block is a little exceptional because customText doesn't have a static default. The default depends on the locale, on the theme, etc. The fact that — prior to this PR — the MoreEdit component already had defaultText: __( 'Read more' ) is probably a mistake.

I'll comment on #23780. Meanwhile, I think this PR should just be about the refactoring. :)

@mcsf mcsf mentioned this pull request Jul 8, 2020
Closed
@ntsekouras
Copy link
Contributor Author

Thanks @mcsf for the elaborate explanation.

I believe we should let the user have an empty text but not show anything default for this PR. If I understand correctly you believe so too, right?

@ZebulanStanphill ZebulanStanphill self-requested a review July 8, 2020 16:34
@ZebulanStanphill
Copy link
Member

Unit test failure is legitimate; the behavior is still different from master, resulting in a different output text and therefore different width value, causing the result to not match the test snapshot.

@ZebulanStanphill
Copy link
Member

It looks like the only remaining test failure is an intermittent/unrelated one. Try rebasing the branch to get the tests to run again.

@ntsekouras ntsekouras force-pushed the refactor/more-block branch from 74f1542 to 8cde812 Compare July 9, 2020 06:13
@ntsekouras ntsekouras merged commit 805bdf8 into master Jul 9, 2020
@ntsekouras ntsekouras deleted the refactor/more-block branch July 9, 2020 06:44
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 9, 2020
@ZebulanStanphill
Copy link
Member

Alright, I've created #23836 to fix the handling of the More block default text placeholder.

@ZebulanStanphill ZebulanStanphill mentioned this pull request Jul 14, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZebulanStanphill ZebulanStanphill ZebulanStanphill approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

@gziolo gziolo Awaiting requested review from gziolo

@mcsf mcsf Awaiting requested review from mcsf

Assignees

@ntsekouras ntsekouras

Labels
[Block] More Affects the More Block - used for displaying the 'Read More' link [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 8.6
Development

Successfully merging this pull request may close these issues.
3 participants
@ntsekouras @ZebulanStanphill @mcsf