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

Apply placeholder on editable wrapper #758

Closed
wants to merge 1 commit into from

Conversation

aduth
Copy link
Member

@aduth aduth commented May 10, 2017

Previously: #756, #475, specifically #475 (comment)

This pull request seeks to simplify the placeholder implementation slightly by moving it up one level to the base Editable component's wrapper. This avoids needing to manage lifecycle to apply attributes to the TinyMCE node directly.

Open questions:

Should the placeholder be centered by default? This makes sense for the image caption, but I'd first experimented with applying it to the heading block, where it felt off (I'd have expected left-aligned). I guess this depends on where we expect placeholders to be used.

When I tried applying placeholder to heading, I found it doesn't always work correctly when removing text from an existing header then focusing out (shown empty instead of with placeholder).

Testing instructions:

There should be no regressions from #756. Notably, you should be able to focus an image block and see a placeholder shown 'til text is entered.

@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label May 10, 2017
@aduth aduth requested a review from ellatrix May 10, 2017 20:12
@ellatrix
Copy link
Member

Like this the placeholder text won't inherit the styles of the editable text. In the PR the text will be bigger than the caption text is. Another problem is, like you said, centring. In master you can add a heading placeholder and it won't centre because styles are inherited.

I'm not sure why we are trying to avoid setting classes on the content editable element. The best way to do this is directly on the element that needs place-holding, otherwise the styles may be wrong, and the positioning may be off. Maybe we just need a better way to allow some prop changes on the element, but not the children?

@jasmussen
Copy link
Contributor

Should the placeholder be centered by default? This makes sense for the image caption, but I'd first experimented with applying it to the heading block, where it felt off (I'd have expected left-aligned). I guess this depends on where we expect placeholders to be used.

Text alignment should be the same as whatever content will be input as you start typing. That is, if you are in a caption, the placeholder text is centered. If you're in a textarea that's left aligned, the placeholder should be left aligned.

Basically what Ella suggests, the placeholder should inherit the style of the field it's holding place for. For example font sizes in captions are 13px, but the placeholder is 16px:

screen shot 2017-05-11 at 10 04 51

In general the pattern we're mimicking is that of the standard input placeholder attribute. That is, the placeholder text is:

  • same font, style, size, weight
  • color the same but lighter

For example placeholder text currently styled for input and textarea is color: $dark-gray-100;.

@jasmussen
Copy link
Contributor

Possibly unrelated to this PR, but there's a little placeholder weirdness with the button now:

screen shot 2017-05-11 at 10 41 40

@ellatrix
Copy link
Member

@jasmussen Yeah I saw that yesterday in master... Will have a look.

@jasmussen
Copy link
Contributor

jasmussen commented May 11, 2017

@iseulde Actually false alarm — I'm working on the button right now. It was lacking a min-width.

Edit hmmm... it would be nice for translation purposes if it worked without a min-width.

@ellatrix
Copy link
Member

@jasmussen min-width can help too here, though I wonder if we should force the placeholder text width if the min-width is smaller. Something low-priority we can explore separately I guess.

@aduth
Copy link
Member Author

aduth commented May 11, 2017

I'm not sure why we are trying to avoid setting classes on the content editable element.

The goal was partly +18 -30, but also that interacting with a DOM node directly directly through React's lifecycle is almost always discouraged if it can be avoided.

But as you've pointed out, I agree it can't be here with the need to inherit styles from the contenteditable.

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.

3 participants