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

Editable: Split caption if empty #2664

Closed
wants to merge 1 commit into from

Conversation

youknowriad
Copy link
Contributor

Fixes #2173

This PR adds a new PR to the Editable component splitIfEmpty which causes the Editable to call onSplit only if it's empty. I'm not totally satisfied but can't think of a better alternative.

This is only implemented for image captions, could be generalized to other captions if we agree on the approach.

@youknowriad youknowriad added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Sep 5, 2017
@youknowriad youknowriad self-assigned this Sep 5, 2017
@codecov
Copy link

codecov bot commented Sep 5, 2017

Codecov Report

Merging #2664 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2664      +/-   ##
==========================================
- Coverage   31.31%   31.26%   -0.05%     
==========================================
  Files         178      178              
  Lines        5429     5437       +8     
  Branches      949      952       +3     
==========================================
  Hits         1700     1700              
- Misses       3154     3159       +5     
- Partials      575      578       +3
Impacted Files Coverage Δ
blocks/editable/index.js 10.27% <0%> (-0.25%) ⬇️
blocks/library/image/block.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da364e8...3960b3b. Read the comment docs.

@aduth
Copy link
Member

aduth commented Sep 6, 2017

Do we even care to support image captions with line breaks?

This isn't precisely what was requested in #2173, where the behavior was suggested to be "move to next focusable", not inserting a new block.

I'll be the dissenting opinion that splitting behavior by whether content exists is awkward and unpredictable. Personally I'd rather we have it so that if the Editable is not explicitly multiline supporting, Enter can proceed to the next focusable, and does so in the WritingFlow component. But making this opt-in or block-specific seems asking for trouble.

@youknowriad
Copy link
Contributor Author

Closing as too complex for this purpose, a more generic solution in WritingFlow could be better as suggested by @aduth.

Also, this is less of an issue since we have arrow navigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants