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: Trailing placeholder attempt2 #4425

Closed
wants to merge 2 commits into from

Conversation

youknowriad
Copy link
Contributor

closes #3078

This PR replaces the trailing inserter with an empty placeholder at the end of the editor.
The differences with the original (reverted) implementation are:

  • Arrow navigation from the title to the placeholder still works
  • The height of the placeholder is equal to the paragraph length
  • If the last block is a paragraph, do not add the trailing placeholder (which solves the multiple empty paragraphs issue)

@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Jan 12, 2018
@youknowriad youknowriad self-assigned this Jan 12, 2018
@youknowriad youknowriad requested a review from jasmussen January 12, 2018 11:17
@youknowriad youknowriad force-pushed the add/trailing-placeholder branch 2 times, most recently from 37ab94e to 2ee822c Compare January 12, 2018 11:34
@jasmussen
Copy link
Contributor

I really really really love this so. Thanks so much for working on this. It feels like such an improvement, and it feels so intuitive in practice.

@mtias made a good point in the past thread, that we still might want to show the inserter in the context of where your caret is. That is, it's not a trailing inserter that's always there, but it's the inline inserter just shown to the left of the placeholder where the movers are. As soon as you start typing, it is replaced by the movers. Essentially what's mocked up here.

It's okay for this behavior to apply to the very first placeholder too.

What do you think?

@youknowriad
Copy link
Contributor Author

@jasmussen The trickiest part is "As soon as you start typing, it is replaced by the movers"

This basically means this inserter should be added in the "paragraph" block itself or have a way to know if the paragraph block is empty (which has been advocated against several times by @aduth)

@jasmussen
Copy link
Contributor

Understood. Can we "cheat"? Can we have an invisible trailing inserter sitting on top of the movers, as part of the block framework itself? And then do some sort of show/hide based on whether the paragraph is empty or not?

@mtias any thoughts?

@youknowriad
Copy link
Contributor Author

@jasmussen That was my idea but we need the isEmpty API for the paragraph block.

@noisysocks
Copy link
Member

This would indadvertedly fix #3793 🙂

@youknowriad youknowriad force-pushed the add/trailing-placeholder branch from 2ee822c to 47f5b07 Compare January 16, 2018 09:12
@youknowriad
Copy link
Contributor Author

@jasmussen Added an inserter (actually moved the sibling inserter) next to the last block. Thoughts?

@jasmussen
Copy link
Contributor

Really appreciate that you keep working on this. Here's a GIF of the latest behavior:

trailing inserter

I appreciate this, but it doesn't fix the issue of the inserter showing only when the placeholder is empty, i.e. it doesn't work when between two paragraphs if you make a linebreak.

How do we know when to show the inserter? The / is only detected when the paragraph is empty.

However this latest effort does surface another point — we have the sibling inserter. Maybe this is sufficient? What if there is no inserter added in this PR, like the previous version, and if you need to insert at the end you have to use the sibling inserter for it?

I've also been mocking up ways to always show the inserter to the left, but so far I'm not feeling it:

screen shot 2018-01-16 at 10 24 52

@jasmussen
Copy link
Contributor

I think getting this PR merged in is an important stepping stone in order to make general improvements to the insertion flow.

I think there is a solution awaiting us, that means always showing the plus next to a block, whether empty or not. This solution, once we find a good design, will also make the sibling inserter moot. As such, it makes sense to move forward with this PR with the minimal amount of work, done, and then do further improvements separately.

The last commit you pushed added the inserter on the side, instead of the movers, on the last block. I think if we can move it so it isn't vertically centered on long blocks (i.e sits where the movers sit) and don't have the fade in/out effect, then I think this is good to go.

@youknowriad
Copy link
Contributor Author

don't have the fade in/out effect, then I think this is good to go

What exactly do you propose here? Always showing it when the block is selected even in typing mode? or just removing the animation? It's not clear with the comment :)

@jasmussen
Copy link
Contributor

Great point. The inserter on the left assumes that it's attached to the literal "trailing placeholder", which wouldn't work both since there isn't one and also since as soon asn you clicked in it it would disappear.

I'm going to check out this branch tomorrow and do a little experiment.

@jasmussen
Copy link
Contributor

In #4539, I basically forked this branch from an earlier version and tried an experiment. Please give it a spin.

@afercia
Copy link
Contributor

afercia commented Jan 17, 2018

Tested a bit, a few considerations:

1
How do I get the block movers for the last block? I couldn't find a way to make them appear, as now there's only the + inserter button on the left of the block

2
when tabbing and focusing the "appender space", the focus indication is very light, almost not noticeable; also, I can't see a caret blinking... not sure why.

screen shot 2018-01-17 at 16 38 04

A stronger focus style and making the caret appear as default in all editable fields maybe could help?

3
Same when hovering with a pointing device: there's no "hover" style and the cursor style is cursor: text; which doesn't help understand that space (actually a button) is clickable. I'd consider some hover style and cursor: pointer;

4
I see there are an input field and a button, both unlabeled. There's no label (or aria-label) for the input. There's no text (or aria-label) for the button.

screen shot 2018-01-17 at 16 36 35

As you know, all UI controls must be labeled with some meaningful text.

5
I'm not sure users will understand the subtleties behind not having the clickable space when the last block is a paragraph. As a user, once I learn that clicking that area adds a block, I'd expect that to always work even when the last block is a paragraph.

@afercia
Copy link
Contributor

afercia commented Jan 17, 2018

I can't see a caret blinking... not sure why.

Seems focus goes to the external wrapper, not on the actual editable element.

@jasmussen
Copy link
Contributor

@afercia I cloned this PR into a new version here: #4425 — it is different in how it handles the inserter, but your feedback might still apply.

@afercia
Copy link
Contributor

afercia commented Jan 18, 2018

I cloned this PR into a new version here: #4425

#4539 ? 🙂

@jasmussen
Copy link
Contributor

Arg yes that one :D

@jasmussen
Copy link
Contributor

Given the version 3 of this has been mered, I think we can close this one. Thanks again Riad.

@youknowriad youknowriad deleted the add/trailing-placeholder branch January 29, 2018 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing the trailing inserter in favor of a clickable area
4 participants