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

Writing Flow: allow split out of caption on Enter #22290

Merged
merged 2 commits into from
May 19, 2020
Merged

Writing Flow: allow split out of caption on Enter #22290

merged 2 commits into from
May 19, 2020

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented May 12, 2020 •
edited
Loading

Description

Kind of ridiculous that I haven't fixed this earlier. Requires a new RichText prop though because the behaviour is quite different from onSplit.

Should it be marked an unstable API at first?
Should Enter from the middle of a caption create a new empty paragraph below? Maybe be a nicer experience.

To do: add this to all other blocks with a caption. Which makes me wonder... should we create a figure-caption component to be used in blocks?

To do: on Backspace from an empty paragraph, the caret should be placed at the end of the caption, but that's an issue in master too.

How has this been tested?

Place the caret at the end of an image caption and press Enter. A new paragraph should be created after the image.

Screenshots

Types of changes

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.

@ellatrix ellatrix added [Type] Enhancement A suggestion for improvement. [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels May 12, 2020
@ellatrix ellatrix requested a review from gziolo May 12, 2020 07:53
@github-actions
Copy link

github-actions bot commented May 12, 2020 •
edited
Loading

Size Change: +88 B (0%)

Total Size: 835 kB

Filename Size Change
build/block-editor/index.js 105 kB +61 B (0%)
build/block-library/index.js 118 kB +27 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 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.19 kB 0 B
build/block-library/editor.css 7.19 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 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.1 kB 0 B
build/components/index.js 182 kB 0 B
build/components/style-rtl.css 17 kB 0 B
build/components/style.css 17 kB 0 B
build/compose/index.js 6.67 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.43 kB 0 B
build/date/index.js 5.47 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.11 kB 0 B
build/edit-navigation/index.js 6.6 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 28.1 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/index.js 7.87 kB 0 B
build/edit-widgets/style-rtl.css 4.69 kB 0 B
build/edit-widgets/style.css 4.69 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 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.64 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 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 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 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 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 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 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@gziolo
Copy link
Member

gziolo commented May 12, 2020

Kind of ridiculous that I haven't fixed this earlier. Requires a new RichText prop though because the behaviour is quite different from onSplit.

It's fine. The most important part is that the proposal is finally there :)

Should it be marked an unstable API at first?

Should it be rather onEnterAtEnd?

Should Enter from the middle of a caption create a new empty paragraph below? Maybe be a nicer experience.

For the blocks that have a single item and the caption as the last item, it could move the content after the cursor to a new block. You can explore it separately.

To do: add this to all other blocks with a caption. Which makes me wonder... should we create a figure-caption component to be used in blocks?

The shared component for caption would be awesome 👍

Super bonus points, Image block could be constructed out of the nested Media (Image variation) and Paragraph (Caption variation) blocks, the container block would have template locking that, doesn't allow to add or remove blocks but maybe you could reorder them :)

@ellatrix
Copy link
Member Author

Should it be rather onEnterAtEnd?

Since we've used split and merge for other props and APIs, I think split might be better? I'm not sure.

For the blocks that have a single item and the caption as the last item, it could move the content after the cursor to a new block. You can explore it separately.

I was thinking about that and tried that first, but to be honest I like the subtle boundary between between the blocks. If we implement text splitting of a caption, we also have to implement merging a paragraph with an image. I'm not sure if that makes sense.

@ellatrix
Copy link
Member Author

Oh I guess onEnter can make sense if it's implemented full by the block and doesn't necessarily add splitting.

@SergioEstevao SergioEstevao self-assigned this May 12, 2020
@SergioEstevao
Copy link
Contributor

Related to: wordpress-mobile/gutenberg-mobile#426

@ellatrix ellatrix mentioned this pull request May 12, 2020
Merged
6 tasks
gziolo
gziolo approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can we get it in as soon as possible? 😄

It's so much better, let's call it unstable and rename later.

I wouldn't mind seeing splitting in the middle as well for the sake of conistency.

SergioEstevao
SergioEstevao previously requested changes May 13, 2020
View reviewed changes
Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

Can we please wait before I review this on GB-mobile? Thanks.

@SergioEstevao
Copy link
Contributor

From my tests this breaks the insertion of multiple lines on the following blocks:

  • pre
  • code
  • verse

On mobile and web.

I'm guessing some props need to be update on the use of RichText component there, or define a default value for the prop that behaves as before.

@ellatrix ellatrix force-pushed the try/split-caption branch from 0f202bb to 195aa2f Compare May 18, 2020 13:47
@ellatrix
Copy link
Member Author

@SergioEstevao Yes, I'm aware of it, the e2e tests did not pass for those. It should be fixed now.

@chrisvanpatten
Copy link
Contributor

Ooh this will be super valuable!

@ellatrix ellatrix dismissed SergioEstevao’s stale review May 19, 2020 11:22

Feedback addressed

@ellatrix ellatrix merged commit 7fa12a3 into master May 19, 2020
@ellatrix ellatrix deleted the try/split-caption branch May 19, 2020 11:23
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 19, 2020
@phpbits phpbits mentioned this pull request Jun 1, 2020
Closed
@SergioEstevao SergioEstevao mentioned this pull request Jun 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@SergioEstevao SergioEstevao SergioEstevao left review comments

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@etoledom etoledom Awaiting requested review from etoledom

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

Assignees

@SergioEstevao SergioEstevao

Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 8.2
Development

Successfully merging this pull request may close these issues.
4 participants
@ellatrix @gziolo @SergioEstevao @chrisvanpatten