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

Writing Flow/Quote: allow splitting #17121

Merged
merged 2 commits into from
Aug 21, 2019
Merged

Writing Flow/Quote: allow splitting #17121

merged 2 commits into from
Aug 21, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Aug 21, 2019

Description

Allows the quote block to be split when the caret is at an empty paragraph. This is similar to how list splitting works. The implementation is really simple.

How has this been tested?

I've added e2e tests.

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.

if ( ! value || value === '<p></p>' ) {
return {
...attributes,
citation: attributes.citation + citation,
citation,
};
}

return {
...attributes,
value: attributes.value + value,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we append the first citation between the two values here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It feels a bit strange to me. :) If you split a quote into two quotes and merge them, you end up with a cite as quote content? In that case I would expect them to get merged in the original form. But maybe that's ok, then it requires a few more backspaces to restore. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I'm not sure either, I guess I "just" wanted to avoid losing that text but it probably doesn't make a lot of sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with trying to avoid data loss. If the merge wasn't good, the user could always revert though. Something else we could do is ask the user which citation to use in case there are two. Kind of like a merge conflict in git. :) This pattern might also be useful to other blocks.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

There are some failing tests, but I like how this feels.

@ellatrix
Copy link
Member Author

Seems to be intermittent. Restarted the tests. :)

@ellatrix
Copy link
Member Author

Always running into this error. No idea what's going wrong.


532$ npm run check-local-changes
533
534> gutenberg@6.3.0 precheck-local-changes /home/travis/build/WordPress/gutenberg
535> npm run docs:build
536
537
538> gutenberg@6.3.0 docs:build /home/travis/build/WordPress/gutenberg
539> node ./docs/tool/index.js && node ./bin/update-readmes.js
540
541
542/home/travis/build/WordPress/gutenberg/docs/tool/update-data.js:48
543			throw stderr.toString();
544			^
545internal/modules/cjs/loader.js:638
546    throw err;
547    ^
548
549Error: Cannot find module 'espree'

@ellatrix
Copy link
Member Author

Did a fresh npm install and this error doesn't occur locally.

@mtias
Copy link
Member

mtias commented Aug 21, 2019

Thanks! This has been a small annoyance for a bit.

@youknowriad
Copy link
Contributor

I think @pento mentioned issues with the last npm release. So it might be related.

@ellatrix ellatrix added the [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... label Aug 21, 2019
@ellatrix
Copy link
Member Author

Only "Build artifacts" is failing because of some issues with npm. I manually verified that there are no build artefacts.

@ellatrix ellatrix merged commit 1aecff9 into master Aug 21, 2019
@ellatrix ellatrix deleted the try/quote-split branch August 21, 2019 21:42
@ellatrix ellatrix added this to the Gutenberg 6.4 milestone Aug 22, 2019
@gziolo
Copy link
Member

gziolo commented Aug 26, 2019

Cool to see this enhancement merged :)

I'd love to see it replicated in all "text" blocks. @ellatrix, do you plan to add it to more blocks? It looks like using this unstable API would allow us to have a couple of working examples before even start thinking about its final shape.

@ellatrix
Copy link
Member Author

Yes, I do, but other normal text field might require a little more work. E.g. #16990 has helped. The unstable API is only for multi line instances. The future of multi line is uncertain which is why it is unstable.

donmhico pushed a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
* Writing Flow/Quote: allow splitting

* Add extra merge e2e test
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Writing Flow/Quote: allow splitting

* Add extra merge e2e test
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Writing Flow/Quote: allow splitting

* Add extra merge e2e test
daniloercoli added a commit that referenced this pull request Sep 4, 2019
 into rnmobile/add-autosave-to-mobile

* 'rnmobile/master' of https://github.com/WordPress/gutenberg: (52 commits)
  [RNMobile] DarkMode improvements (#17309)
  Remove redundant bg color within button appender (#17325)
  Support group block on mobile (#17251)
  [RNMobile] Insure tapping at end of post inserts at end
  Recover border colors (#17269)
  [RNMobile] Fix dismiss keyboard button for the post title (#17260)
  Unify media placeholder and upload props within media-text (#17268)
  MediaUpload and MediaPlaceholder unify props (#17145)
  Add native support for the MediaText block (#16305)
  Activate Travis CI on rnmobile/master branch (#17229)
  [RNMobile] Native mobile release v1.11.0 (#17181)
  Apply box-sizing border-box properly to the notices components (#17066)
  Writing Flow: allow undo of patterns with BACKSPACE and ESC (#14776)
  Project automation: Rewrite actions using JavaScript (#17080)
  Build: remove global install of latest npm since we want to use the paired node/npm version (#17134)
  Writing Flow/Quote: allow splitting (#17121)
  Use `400` as a valid `font-weight`
  Add: Disabled block count in the block manager (#17103)
  Update video player style on mobile - Add a new gridicon play icon, from: https://github.com/Automattic/gridicons/blob/87c9fce08b4a9f184b9fb4963228757fdd4f4e74/svg-min/gridicons-play.svg - Replace the Dashicon play by this one - Update icon size and icon color - Update the overlay color
  [RNMobile] Hide replaceable block when adding block (#16931)
  ...

# Conflicts:
#	packages/block-editor/src/components/block-list/index.native.js
#	packages/block-editor/src/components/inserter/index.native.js
#	packages/block-editor/src/components/inserter/menu.native.js
#	packages/block-editor/src/components/media-placeholder/index.native.js
#	packages/block-editor/src/components/warning/index.native.js
#	packages/block-library/src/code/edit.native.js
#	packages/block-library/src/image/edit.native.js
#	packages/block-library/src/missing/edit.native.js
#	packages/block-library/src/more/edit.native.js
#	packages/block-library/src/nextpage/edit.native.js
#	packages/block-library/src/video/edit.native.js
#	packages/components/src/mobile/bottom-sheet/cell.native.js
#	packages/components/src/mobile/bottom-sheet/index.native.js
#	packages/components/src/mobile/dark-mode/index.native.js
#	packages/components/src/mobile/html-text-input/index.native.js
#	packages/components/src/toolbar/toolbar-container.native.js
#	packages/edit-post/src/components/header/header-toolbar/index.native.js
#	packages/edit-post/src/components/layout/index.native.js
#	packages/edit-post/src/components/visual-editor/index.native.js
#	packages/rich-text/src/component/index.native.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Quote Affects the Quote Block [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants