Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

t/ckeditor5/1517: The editor placeholder configuration using the placeholder attribute should be restricted to <textarea> only #84

Merged
merged 2 commits into from
Feb 11, 2019

Conversation

oleq
Copy link
Member

@oleq oleq commented Feb 11, 2019

Suggested merge commit message (convention)

Fix: The editor placeholder configuration using the placeholder attribute should be restricted to <textarea> only (see ckeditor/ckeditor5#1517).


Additional information

Part of ckeditor/ckeditor5#1518.

@coveralls
Copy link

coveralls commented Feb 11, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 53f9877 on t/ckeditor5/1517 into c90c23a on master.

@oleq oleq changed the title t/ckeditor5/1517: Used the data-placeholder attribute instead of invalid placehold… t/ckeditor5/1517: The editor placeholder configuration using the placeholder attribute should be restricted to <textarea> only Feb 11, 2019

const placeholderText = editor.config.get( 'placeholder' ) ||
editor.sourceElement && editor.sourceElement.getAttribute( 'placeholder' );
sourceElement && sourceElement.tagName.toLowerCase() === 'textarea' && sourceElement.getAttribute( 'placeholder' );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sourceElement && sourceElement.tagName.toLowerCase() === 'textarea' && sourceElement.getAttribute( 'placeholder' );
sourceElement && sourceElement.tagName === 'TEXTAREA' && sourceElement.getAttribute( 'placeholder' );

Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with the rest of our codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Or not... we've got both. So I'm leaving this as it is in your PR.

@Reinmar
Copy link
Member

Reinmar commented Feb 11, 2019

BTW, all those changes are internal because they fix something that was introduced in the same iteration.

@Reinmar Reinmar merged commit 7547a23 into master Feb 11, 2019
@Reinmar Reinmar deleted the t/ckeditor5/1517 branch February 11, 2019 14:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants