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

Verse: Disable line breaks #52783

Merged
merged 2 commits into from Jul 21, 2023
Merged

Verse: Disable line breaks #52783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5716fda
Verse: Disable line breaks
Mamaduka Jul 20, 2023
821c5bd
Adjust mobile unit tests
Mamaduka Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/block-library/src/verse/edit.js
View file
Open in desktop
Expand Up @@ -13,13 +13,15 @@ import {
AlignmentToolbar,
useBlockProps,
} from '@wordpress/block-editor';
import { createBlock, getDefaultBlockName } from '@wordpress/blocks';

export default function VerseEdit( {
attributes,
setAttributes,
mergeBlocks,
onRemove,
style,
insertBlocksAfter,
} ) {
const { textAlign, content } = attributes;
const blockProps = useBlockProps( {
Expand Down Expand Up @@ -56,6 +58,9 @@ export default function VerseEdit( {
textAlign={ textAlign }
{ ...blockProps }
__unstablePastePlainText
__unstableOnSplitAtEnd={ () =>
insertBlocksAfter( createBlock( getDefaultBlockName() ) )
}
/>
</>
);
Expand Down
6 changes: 4 additions & 2 deletions packages/block-library/src/verse/test/edit.native.js
View file
Open in desktop
Expand Up @@ -64,13 +64,15 @@ describe( 'Verse block', () => {
const verseTextInput = await screen.findByPlaceholderText(
'Write verse…'
);
typeInRichText( verseTextInput, 'A great statement.' );
typeInRichText( verseTextInput, 'A great statement.Again', {
finalSelectionStart: 18,
finalSelectionEnd: 18,
} );
Comment on lines +67 to +70
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've adjusted the tests. Took inspiration for Pullquote tests, where citation also creates a new default block.

gutenberg/packages/block-library/src/pullquote/test/edit.native.js

Lines 48 to 56 in 8aa016e

typeInRichText( citationTextInput, 'A person', {
finalSelectionStart: 2,
finalSelectionEnd: 2,
} );
fireEvent( citationTextInput, 'onKeyDown', {
nativeEvent: {},
preventDefault() {},
keyCode: ENTER,
} );

Copy link
Member

Choose a reason for hiding this comment

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

The original spirit of this test was to assert the outcome of pressing Return at the end of lines. While this test change results in the snapshot remaining the same and the test passing, the change here communicates that a user might...

  1. Type A great statement.Again,
  2. Move their cursor between statement. and Again.1,
  3. And press the Return key.

While this is possible, this behavior may be too cumbersome to be the only way to insert line breaks in the Verse block when using the mobile editor. There is no ability to simultaneously press Shift and Return on most mobile OS virtual keyboards.

If needed, we could diverge the mobile editor behavior to avoid this cumbersome outcome, i.e. introduce a packages/block-library/src/verse/edit.native.js. Hopefully, we could identify common logic to continue sharing between the two files.

However, before doing so, I wanted to pose a question: how should pressing the Return key behave in the Preformatted block? Should Preformatted behave like a "container" block or should it align with Verse, Header, Paragraph? From my testing, Preformatted currently behaves like the former (as Verse did prior to this PR).

Footnotes

  1. This cursor movement is communicated in the form of the finalSelection[Start|End] options for typeInRichText. In light of this this misunderstanding of the test behavior, it may be best to remove/privatize these options and instead rely upon the explicit selectRangeInRichText. ↩

Copy link
Member

Choose a reason for hiding this comment

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

Given that the Verse block is for content like poetry and lyrics, line breaks should be expected.

There is no ability to simultaneously press Shift and Return on most mobile OS virtual keyboards.

I agree that moving the cursor position is too cumbersome to be the only way to create a line break within a Verse block on mobile. As a possible alternative, we could consider a soft return, where pressing the shift key and then pressing enter on mobile creates a new line within the Verse block. Soft returns are a common pattern in multiline text input on mobile (like messaging apps) that may already be intuitive to users. A hard return (the behavior implemented in this PR) could create a new block, which would minimize divergence between web and mobile behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the great feedback. @stokesman had the same concerns. It's clear to me now that users expect line breaks when pressing Enter or Return in the Verse block.

It would be easier to revert the changes and mark #52773 as wontfix for now.

Should Preformatted behave like a "container" block, or should it align with Verse, Header, Paragraph? From my testing, Preformatted currently behaves like the former (as Verse did prior to this PR).

The container blocks support this feature differently when two consecutive Enters create a new block. Maybe we can adopt similar logic for some non-container blocks as well.

Cc @annezazu, since you extracted the original issue from the GitHub discussions.

Copy link
Contributor

Choose a reason for hiding this comment

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

If reverting this change on the web is an option, I'd like to add support for that, as I agree with sentiments already shared that users will be expecting/wanting line breaks in the Verse block.

The container blocks support this feature differently when two consecutive Enters create a new block. Maybe we can adopt similar logic for some non-container blocks as well.

I'd also be wary of adding this as a solution as folks may want the freedom of adding blank lines between content in multiline blocks. In the case of the Verse block, for example, a poem may include multiple stanzas that should be separated with a blank line.

I'm afraid I don't have any bright ideas for solving the original problem of more easily exiting the Verse block. Perhaps after hitting return twice, the toolbar could emerge, so that the option of adding a new block is more readily available if users want to take it? Or perhaps there's some other creative approach that could be taken? 🤔 It would be interesting to figure out whether this is a common issue that those using multiline blocks face.

Copy link

Choose a reason for hiding this comment

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

Perhaps after hitting return twice

That's exactly what I have would expected. Three times actually.

  1. Writing: I am writing a verse Enter
  2. Still in the verse Thinking: I want a blank paragraph in between. Enter
  3. Still in the verse Writing: I write the last paragraph in the verse.Enter
  4. Still in the verse Thinking: Blank paragraph, as above Enter
  5. Still in the verse Thinking: Okay, if I type now it'd be the same as step 3. But I don't type, so let's get out of here. Enter
  6. Out of the verse

Copy link

@hanneslsm hanneslsm Jul 25, 2023 •
edited

Choose a reason for hiding this comment

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

To put it simply: It's not possible to create two blank paragraphs after each other. (At the end of the verse block)

Copy link

Choose a reason for hiding this comment

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

Ah, I see that @jameskoster suggested the same below and it can' t be done right now.
However, this should be definitely be done in the future. Is anyone aware of a ticket, or should we utilise this one: #52773 @Mamaduka Could you please reopen it again?

fireEvent( verseTextInput, 'onKeyDown', {
nativeEvent: {},
preventDefault() {},
keyCode: ENTER,
} );
typeInRichText( verseTextInput, 'Again' );

// Assert
expect( getEditorHtml() ).toMatchInlineSnapshot( `
Expand Down