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

Fix selection of small table cells #9673

Merged
merged 1 commit into from
Sep 10, 2018
Merged

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Sep 6, 2018

Description

I noticed that it can be difficult to select very narrow table cells. The RichText element within the table cell is the element that receives selection. In the gif below the rich text has collapsed to just 1px wide and is hard to select:
table-cell-selection

This PR moves padding from the td element to the RichText (just for the editor) so that there's a larger selection area.

Pretty open to other suggestions of how to fix this - this seemed like the simplest.

How has this been tested?

Manually tested selection of a narrow cell

Screenshots

Gif showing the fix
table-cell-selection-with-fix

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@talldan talldan added [Type] Bug An existing feature does not function as intended [Feature] Blocks Overall functionality of blocks labels Sep 6, 2018
@talldan talldan added this to the 3.9 milestone Sep 6, 2018
@talldan talldan self-assigned this Sep 6, 2018
@talldan talldan requested a review from a team September 6, 2018 18:57
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It looks good from me the code perspective. I would be happy to have it confirmed from one of the designers, too.

@@ -16,4 +16,8 @@
box-shadow: inset 0 0 0 1px $blue-medium-500;
border-style: double;
}

&__cell-content {
Copy link
Member

Choose a reason for hiding this comment

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

@jasmussen - do we have any pattern for the placeholder in the table cell?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasmussen Just thought I'd ping you again on this question in case you missed it. Cheers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope you don't mind, I've also added you as a reviewer based on Grzegorz' feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I missed the ping!

I think PR works well for me!

We don't have any pattern for placeholder text in tables, and I don't think we need it either. The table resembles a table in its natural state, and it's clear where you need to set the caret in order to type. Also, given table cells can grow very small, as this PR is about, it would be hard to add placeholder text that worked within those rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome - thanks for the reviews on this! 😃

Will merge now.

@gziolo gziolo added the Needs Design Feedback Needs general design feedback. label Sep 7, 2018
@talldan talldan requested a review from jasmussen September 10, 2018 14:37
@jasmussen jasmussen removed the Needs Design Feedback Needs general design feedback. label Sep 10, 2018
@talldan talldan merged commit f5338ed into master Sep 10, 2018
@talldan talldan deleted the fix/table-cell-selection branch September 10, 2018 15:46
talldan added a commit that referenced this pull request Jan 25, 2019
talldan added a commit that referenced this pull request Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants