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

Try: Make it easier to select the Separator #13695

Closed
wants to merge 1 commit into from

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Feb 6, 2019

Fixes #12080

The separator is usually very thin, almost by definition. This makes it hard to select.

This PR tries to make the hit area for the block bigger, therefore easier to select.

It does so at the cost of a little overlap — but this seems worth it. What are your thoughts?

Before (barely selectable):

before

After:

after

Overlap:

overlap

@jasmussen jasmussen added the [Type] Enhancement A suggestion for improvement. label Feb 6, 2019
@jasmussen jasmussen self-assigned this Feb 6, 2019
@jasmussen jasmussen requested review from kjellr and a team February 6, 2019 11:34
@kjellr
Copy link
Contributor

kjellr commented Feb 6, 2019

Thanks for the exploration. I don't love the overlapping borders, but I do like that I can actually select the Separator block for once. 😄

There's actually plenty of space inside the separator block to click today... the problem is that clicking it doesn't actually do anything:

screen-recording-2019-02-06-at-2 20

It seems that there's some sort of buffer margin around the clickable part — is it possible to override that for this block and just make everything inside of the blue border clickable?

@jasmussen
Copy link
Contributor Author

There's actually plenty of space inside the separator block to click today... the problem is that clicking it doesn't actually do anything:

Solid review, thanks for bringing that up. I actually meant to comment on this in the PR but completely forgot to.

To do that now — yes, we should make that area clickable. I keep pinging @aduth forgetting which issue we discussed this on, and here I did it again. Apologies Andrew, please ignore this if you're busy.

Here's what's going on:

  • You can click the padding left and right of the block.
  • When you click the padding above and below, you're actually clicking the block appender — the "inbetweenserter", it captures the click even though it maybe shouldn't. This is because there's a big wide area that you can hover to surface the actually clickable button in the center.

The way to fix this is to make sure to propogate a click there to the block below. Let me see if I can create a ticket for that.

Perhaps we should put this PR on pause awaiting thoughts on the ticket?

@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

Does it try to fix #12080? There is also #12950 PR opened which tries to attack this issue.

@jasmussen
Copy link
Contributor Author

Correct, it does fix that issue.

Given Kjell's feedback, do you think we should get this PR in as a stopgap improvement, or should we wait for #13723 to be fixed?

@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

I think we should land it as a temporary solution and work on #13723 afterward.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

I think we should land it as a temporary solution and work on #13723 afterward.

Agreed. Let's get this in for now, as it's near-impossible to select the block in the meantime. 🙂 But I'd love to see #13723 implemented sooner rather than later.

Looks like this just needs a small conflict fixed, and then it should be good to go.

The separator is usually very thin, almost by definition. This makes it hard to select.

This PR tries to make the hit area for the block bigger, therefore easier to select.

It does so at the cost of a little overlap — but this seems worth it. What are your thoughts?
@jasmussen jasmussen force-pushed the try/make-separator-easier-to-select branch from bfed72b to d205733 Compare February 8, 2019 08:01
@jasmussen
Copy link
Contributor Author

I rebased and tests should pass, however I discovered an issue with one of the variations so please hold off on merging this right at this moment, want to verify it's fixable.

Sidenote, I've noticed that whenever code changes, it automatically requests new reviews. I haven't actually myself requested additional reviews here. That's fine, I suppose, but worth knowing for reviewers that get inundated with requests.

@gziolo
Copy link
Member

gziolo commented Feb 8, 2019

Sidenote, I've noticed that whenever code changes, it automatically requests new reviews. I haven't actually myself requested additional reviews here. That's fine, I suppose, but worth knowing for reviewers that get inundated with requests.

No worries, it all works as expected. GitHub handles it behind the scenes to ensure that the proper group of reviewers is notified as soon as the file in the folder they watch gets updated :)

See #13604 to learn more. You can still add yourself to the list if you want to get such notifications, one folder where designers might be very helpful with their reviews is packages/components. Just explaining how it works :)

@jasmussen jasmussen added the [Status] In Progress Tracking issues with work in progress label Feb 8, 2019
@jasmussen
Copy link
Contributor Author

Sigh, I thought I had it. I thought I had a solution that made the clickable area of the separator bigger, without affecting a theme's ability to style the separator. Turns out that was not the case, as the "dots" variation was affected badly by this.

I tried a bunch of things, but in the end all of them were super hacky, so it seems the real solution is #13723. :|

Closing this one :(

@jasmussen jasmussen closed this Feb 8, 2019
@jasmussen jasmussen removed this from the 5.1 (Gutenberg) milestone Feb 8, 2019
@ashwin-pc ashwin-pc mentioned this pull request Apr 6, 2019
5 tasks
@youknowriad youknowriad deleted the try/make-separator-easier-to-select branch May 27, 2020 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants