Add subscript and superscript feature #74

idleb · 2018-08-29T11:26:27Z

I noticed that basic-styles look as if coreStyles in CKEditor4, but I didn't find subscript and superscript features, so I added them like CKEditor4

coveralls · 2018-08-29T11:32:01Z

Coverage remained the same at 100.0% when pulling 35eb821 on idleb:master into 53ffd41 on ckeditor:master.

ma2ciek · 2018-08-29T12:27:37Z

Hi @idleb!

Thanks for the PR.

Could you tell us:

What are your use cases for this feature?
Why do you think this feature is important?

We usually start by creating issues, so we can deeply discuss use cases for every feature and code change in our codebase. Note, that CKE4 and CKE5 are different editors, they have a little bit different usage and their features don't match 1-1.

In the past, we considered  and  elements being not necessary (see ckeditor/editor-recommendations#5). But I see that the W3C recommendations for this script might have changed as the links in the above ticket are inactive. Now there's only a link to both features ( https://www.w3.org/TR/html5/textlevel-semantics.html#the-sub-and-sup-elements) with the following information:

In general, authors should use these elements only if the absence of those elements would change the meaning of the content.

idleb · 2018-08-30T02:31:50Z

Hi @ma2ciek ,I just saw that about  and  issues.

First, I am migrating from CKEditor4, and latest editor used this features for mathematical equation. because our product usually was used by teachers in school, e.g. print test paper. So we need these styles.

I think use sub and sup is more easily, but you are right about it's not a general function. Maybe, I should implement them in a new package.

Reinmar · 2018-08-30T16:08:31Z

I don't see it a problem that Editor Recommendations don't consider sub/sup as semantic enough. ER didn't mean to define the set of features that we can never exceed. It was meant to define a basic set of "Article" oriented features. We already extended that set because there are other use cases than creating articles in CKEditor 5.

BTW, even in this package we have underline and code (and they are not covered by ER). We can easily have sub/sup too.

So, I'd really like to see a progress with sub/sup. @idleb, if you could reopen the PR, that'd be cool. It adds a very basic sub/sup functionality and that's ok for now.

But, I agree with @ma2ciek on one thing – we need to discuss how this feature should be provided. There are things like... can text be in  and  at the same time? If yes, which of these elements comes first (which one is outside)? How to implement it to achieve such behaviour (I can tell that it won't be that easy if these attrs are to be ordered)?

idleb · 2018-08-31T02:04:49Z

@Reinmar ,I am glad you asked me to reopen the PR, this feature is very basic. I don't consider about text is in _{and ^{at the same time, which one is outside depend on how user typesetting.}}

Reinmar · 2018-08-31T15:59:41Z

OK, I reported the ticket about the behaviour of this feature: https://github.com/ckeditor/ckeditor5-basic-styles/issues/75

ma2ciek · 2018-09-10T11:22:30Z

src/subscript/subscriptui.js

+		const t = editor.t;
+
+		// Add subscript button to feature components.
+		editor.ui.componentFactory.add( SUB, locale => {


The UI item should be named subscript, not sub.

ma2ciek · 2018-09-10T11:23:05Z

src/subscript/subscriptediting.js

+		} );
+
+		// Create sub command.
+		editor.commands.add( SUB, new AttributeCommand( editor, SUB ) );


The subscript command should be named subscript, not sub.

ma2ciek · 2018-09-10T11:23:24Z

src/superscript/superscriptediting.js

+		} );
+
+		// Create super command.
+		editor.commands.add( SUPER, new AttributeCommand( editor, SUPER ) );


Same as above

ma2ciek · 2018-09-10T11:23:35Z

src/superscript/superscriptui.js

+			view.bind( 'isOn', 'isEnabled' ).to( command, 'value', 'isEnabled' );
+
+			// Execute command.
+			this.listenTo( view, 'execute', () => editor.execute( SUPER ) );


Same as above

ma2ciek · 2018-09-10T11:51:54Z

Hi again, @idleb.

Sorry for the late review.

I've tested it and it works fine, good job 👍. With the current implementation, you can have both subscript and superscript, but for now, it's ok.

Two things are missing now:

updated contexts.json file (https://github.com/ckeditor/ckeditor5-basic-styles/blob/master/lang/contexts.json) with translation labels
updated manual test, so we can test it live (https://github.com/ckeditor/ckeditor5-basic-styles/tree/master/tests/manual)

It'd be great if you could add these missing things and change the naming of UI items and commands that are mentioned in the review.

And the last thing - do you have permissions to use these icons? Did you create them or found them somewhere?

idleb · 2018-10-12T03:16:53Z

Hello, I updated you mentioned in the review, the feature name, contexts.json and manual test.

About these icons, they was used temporary, I found them from otherwhere. I would need a SVG icon for the default theme.

@idleb

Feature: Introduced the `Superscript` and the `Subscript` features. Closes #76. Closes #74. Thanks to @idleb!

Reinmar · 2018-11-13T17:02:18Z

We merge the feature in #80. Thank you, @idleb!

kylane · 2018-11-13T23:42:50Z

Thanks guys!

add subscript and superscript feature

da45e46

fix tests

de199b5

idleb closed this Aug 30, 2018

idleb reopened this Aug 31, 2018

Reinmar changed the title ~~add subscript and superscript feature~~ Add subscript and superscript feature Aug 31, 2018

Reinmar requested a review from ma2ciek August 31, 2018 15:59

ma2ciek reviewed Sep 10, 2018

View reviewed changes

Reinmar added the status:review- label Sep 14, 2018

idleb added 5 commits October 12, 2018 10:36

update

230d97c

update tests

9b8231f

update

b5c65e9

Merge branch 'master' of https://github.com/idleb/ckeditor5-basic-styles

da032b1

update

35eb821

ma2ciek mentioned this pull request Nov 2, 2018

Introduced the superscript and the subscript features #80

Merged

Reinmar closed this in #80 Nov 13, 2018

Reinmar added a commit that referenced this pull request Nov 13, 2018

Merge pull request #80 from ckeditor/t/76

f281e99

Feature: Introduced the `Superscript` and the `Subscript` features. Closes #76. Closes #74. Thanks to @idleb!

Reinmar mentioned this pull request Oct 9, 2019

Behaviour of sub/sup features ckeditor/ckeditor5#5556

Open

ma2ciek mentioned this pull request Oct 9, 2019

Create basic implementation of sub/sup features ckeditor/ckeditor5#5557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add subscript and superscript feature #74

Add subscript and superscript feature #74

idleb commented Aug 29, 2018

coveralls commented Aug 29, 2018 •

edited

Loading

ma2ciek commented Aug 29, 2018

idleb commented Aug 30, 2018

Reinmar commented Aug 30, 2018

idleb commented Aug 31, 2018

Reinmar commented Aug 31, 2018

ma2ciek Sep 10, 2018

ma2ciek Sep 10, 2018

ma2ciek Sep 10, 2018

ma2ciek Sep 10, 2018

ma2ciek commented Sep 10, 2018 •

edited

Loading

idleb commented Oct 12, 2018

Reinmar commented Nov 13, 2018

kylane commented Nov 13, 2018

Add subscript and superscript feature #74

Add subscript and superscript feature #74

Conversation

idleb commented Aug 29, 2018

coveralls commented Aug 29, 2018 • edited Loading

ma2ciek commented Aug 29, 2018

idleb commented Aug 30, 2018

Reinmar commented Aug 30, 2018

idleb commented Aug 31, 2018

Reinmar commented Aug 31, 2018

ma2ciek Sep 10, 2018

Choose a reason for hiding this comment

ma2ciek Sep 10, 2018

Choose a reason for hiding this comment

ma2ciek Sep 10, 2018

Choose a reason for hiding this comment

ma2ciek Sep 10, 2018

Choose a reason for hiding this comment

ma2ciek commented Sep 10, 2018 • edited Loading

idleb commented Oct 12, 2018

Reinmar commented Nov 13, 2018

kylane commented Nov 13, 2018

coveralls commented Aug 29, 2018 •

edited

Loading

ma2ciek commented Sep 10, 2018 •

edited

Loading