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

EZP-26516: As an editor, I want to be able to add/edit tabular data in RichText editor #772

Merged
merged 9 commits into from Jun 13, 2017

Conversation

4 participants
@andrerom
Copy link
Member

andrerom commented Jan 3, 2017

Issue: https://jira.ez.no/browse/EZP-26516

Todo:

  • Add Tests
  • Translation string
  • Disable Embed and Image inside table for now (logic for inline toolbar to cover several elements at the same time like table+embed is for another larger story later)
  • Table styling
  • Verify if table heading work in all options exposed by alloy (works, even scope attribute for screen readers seems to work)

Screenshots (As of April)

screen shot 2017-04-29 at 15 58 28
screen shot 2017-04-29 at 15 58 47
screen shot 2017-04-29 at 16 19 03
screen shot 2017-04-29 at 16 19 16
screen shot 2017-04-29 at 16 13 34

@andrerom andrerom changed the title WIP: Adding Table to Editor [WIP] EZP-26516: As an editor, I want to be able to add/edit tabular data in RichText editor Jan 3, 2017

@andrerom andrerom force-pushed the eztable branch from 1f72811 to 1e47c31 Jan 3, 2017

@andrerom andrerom changed the base branch from master to 1.7 Jan 3, 2017

@rolandbenedetti

This comment has been minimized.

Copy link

rolandbenedetti commented Jan 3, 2017

From my perspective, it's good to keep it like this for now.

@andrerom

This comment has been minimized.

Copy link
Member Author

andrerom commented Jan 4, 2017

Ok, but I'd still like to adjust the style of the table.

@dpobel anything else you'd expect here? (besides test, and translation string) And if you agree with adjusting color of borders on tables, where should that be placed?

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Jan 4, 2017

@andrerom indeed the table deserves some styling. You should probably do the same thing as for embed ie add a CSS module richtext-table.css (so 2 CSS file, one for layout, one for theme). But unlike for embed, you can not just target table elements in this CSS, so I guess you should make sure both the richtext field view and field edit views generate an element with the same class (I can see that RichText field view has a div.ez-richtext-content, maybe the field edit view should have the class ez-richtext-content class added somewhere and then you can style the table based on this class).

Code wise, that seems ok (besides minor CS problems) but by quickly testing I'm more worried about what's happening when you put some content in the table. For instance, if you add an embed in a cell, you don't have the toolbar to manage it, so you can not do anything (you can not even remove it!). Also on embed in table cell, it seems like there's an additional paragraph in the cell that can not be removed (for others elements it seems to be possible)

@andrerom andrerom changed the base branch from 1.7 to master Jan 4, 2017

@andrerom andrerom force-pushed the eztable branch 8 times, most recently from 733658c to 52a525e Apr 28, 2017

@andrerom

This comment has been minimized.

Copy link
Member Author

andrerom commented Apr 29, 2017

PR updated:

  • embed & image button disabled while inside table as suggested by @rolandbenedetti
  • some styling added also to make table look flat + padding for usability
  • added table heading menu from alloy given we support that as of v1.9 in RichText parsers

@dpobel / @supriyabhargava / @SylvainGuittard / @rolandbenedetti See updated images above for what current status is for the optimal use cases. When it comes to remaining not so optimal use case be aware that it is sometimes possible to insert headings (and paragraphs, but less of an issue) in the table (normally it is created after the table, but in some cases inside, unsure why):
screen shot 2017-04-29 at 16 29 57

But from the looks of the online AlloyEditor Demo, it seems they have added a toolbar to be able to set it back to normal:
<image removed as that is not a topic here, it's for follow up story once we update alloy>

So should we aim to disable heading button also, or aim to update alloy as a followup? wdyt @dpobel?

@rolandbenedetti

This comment has been minimized.

Copy link

rolandbenedetti commented May 1, 2017

Hello André,

Congrats, honestly may be not perfect, but that would be a great move forward in my opinion.
It's great to have the Alloy table's headers dropdown supported.

About the new styling dropdown, I think it would be great to also be able to use it. It gets me a bit confused though. On one end, it allows to choose the whole style of a table (I guess under Object styles) which I think can be very useful especially if developers can easily define such styles, on the other it acts on one single cell, allowing to update the style of a cell's content to normal. Once done, I see no way to move back to the heading style. But beside that, I'd support adding this as well.

+1 for me

@andrerom

This comment has been minimized.

Copy link
Member Author

andrerom commented May 3, 2017

About the new styling dropdown, I think it would be great to also be able to use it. It gets me a bit confused though. ...

As said I won't be able to add that here, so we can deal with that in followup on how it should behave. We most likely don't support the given style choices, so only thing I found clearly relevant without needs for additional backend features (also potentially thinning out our content vs design argument), is the option to normalize style.

Anyway, not relevant here, lets discuss those concepts separately as part of effort to update alloy from 1.2.5 to 1.4.0 (or higher once we do it). It's a different story.

@SylvainGuittard

This comment has been minimized.

Copy link

SylvainGuittard commented May 4, 2017

Impressive @andrerom. Good job.

About the new styling dropdown, I think it would be great to also be able to use it. It gets me a bit confused though. On one end, it allows to choose the whole style of a table (I guess under Object styles) which I think can be very useful especially if developers can easily define such styles, on the other it acts on one single cell, allowing to update the style of a cell's content to normal. Once done, I see no way to move back to the heading style. But beside that, I'd support adding this as well.

I totally agree. Normal is very confusing. I would love to have this styling option and maybe hide the Normal option.

@rolandbenedetti

This comment has been minimized.

Copy link

rolandbenedetti commented May 4, 2017

lets discuss those concepts separately as part of effort to update alloy from 1.2.5 to 1.4.0 (or higher once we do it). It's a different story.

Absolutely !

@andrerom andrerom force-pushed the eztable branch 2 times, most recently from caa6423 to e543eb5 May 24, 2017

@andrerom andrerom force-pushed the eztable branch from d19a99c to a965ba2 Jun 3, 2017

@andrerom andrerom requested review from dpobel and StephaneDiot Jun 5, 2017

@andrerom andrerom force-pushed the eztable branch from 3185306 to db5dec5 Jun 6, 2017

// http://docs.ckeditor.com/#!/api/CKEDITOR.dom.elementPath
// There is also isContextFor( tag ), so if there is a way to specify where embeds
// are valid that would potentially be cleaner
this.state.isDisabled = path && path.contains('table', true) !== null;

This comment has been minimized.

Copy link
@dpobel

dpobel Jun 9, 2017

Contributor

this is forbidden https://facebook.github.io/react/docs/state-and-lifecycle.html#do-not-modify-state-directly ;)
also as you wrote in the comment above, the is disabled logic should not be here but rather in the embed plugin. That said, I'm unsure how you can access the embed plugin for here, that's a bit far away in my head.

This comment has been minimized.

Copy link
@andrerom

andrerom Jun 11, 2017

Author Member

Ok, then I should not use state at all, this is already called during render(), no point causing recursive rendering.

What I was trying to avoid was isDisabled() having to be called two times by design, from getStateClasses() and render().

Input on how I can avoid welcome, but for next commit I'll get rid of this.

This comment has been minimized.

Copy link
@andrerom

andrerom Jun 11, 2017

Author Member

also as you wrote in the comment above, the is disabled logic should not be here but rather in the embed plugin.

I wasn't saying embed plugin, but maybe indeed that is where logic for isContextFor would have to live. But for now I'll cleanup the comment.

@andrerom andrerom force-pushed the eztable branch from 976292d to c9235b0 Jun 11, 2017

@andrerom andrerom changed the title [WIP] EZP-26516: As an editor, I want to be able to add/edit tabular data in RichText editor EZP-26516: As an editor, I want to be able to add/edit tabular data in RichText editor Jun 11, 2017

@andrerom andrerom force-pushed the eztable branch from c9235b0 to f7ae3a0 Jun 11, 2017

@dpobel
Copy link
Contributor

dpobel left a comment

comments in embed.jsx also apply to image.jsx

* @method isDisabled
* @return {Boolean} True if the command is disabled, false otherwise.
*/
isDisabled: function () {

This comment has been minimized.

Copy link
@dpobel

dpobel Jun 12, 2017

Contributor

it should be protected (ie starts with a _ and be documented as protected)

This comment has been minimized.

Copy link
@andrerom

andrerom Jun 12, 2017

Author Member

could do that, but then it won't work in getStateClasses() as expected by ButtonStateClasses

ref: https://github.com/liferay/alloy-editor/blob/25151898f9385f09096f05ab7cf30aeea281330d/src/ui/react/src/components/base/button-state-classes.js

This comment has been minimized.

Copy link
@dpobel

dpobel Jun 12, 2017

Contributor

good point :)


// http://docs.ckeditor.com/#!/api/CKEDITOR.dom.elementPath
// There is also isContextFor( tag ) which potential could have been used.
return path && path.contains('table', true) !== null;

This comment has been minimized.

Copy link
@dpobel

dpobel Jun 12, 2017

Contributor

quickly tried to put that in a re-usable API to avoid duplication and here is what can be done:
in Resources/public/js/alloyeditor/plugins/embed.js you can write something like:

CKEDITOR.plugins.add('ezembed', {                                           
    requires: 'widget,ezaddcontent',                                                                                                                                                                                                       
                                                                                                                                                                                                                                               
    init: function (editor) {
        editor.ezembed = {
            canBeAdded: function () { // feel to suggest a better name
                var path = editor.elementPath();

                return path && path.contains('table', true) !== null;
            }
        }

        editor.widgets.add('ezembed', {
        // ...
    },

and here then you can write:

_isDisabled: function () {
    return this.props.editor.get('nativeEditor').ezembed.canBeAdded();
}

This comment has been minimized.

Copy link
@andrerom

andrerom Jun 12, 2017

Author Member

Done in 4681fe9 (with adjusted logic for the verb used here)

@dpobel

dpobel approved these changes Jun 12, 2017

Copy link
Contributor

dpobel left a comment

(don't forget to remove the TMP Revert commit, @StephaneDiot merged the PR fixing BDD tests this morning)

@andrerom andrerom force-pushed the eztable branch from 4681fe9 to c757944 Jun 12, 2017

@andrerom

This comment has been minimized.

Copy link
Member Author

andrerom commented Jun 13, 2017

Merging as QA is off today, they'll have to bombard this as part of the release tarball as of tomorrow.

@andrerom andrerom merged commit 27faf86 into master Jun 13, 2017

5 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ezrobot/csslint Code review by ezrobot
Details
ezrobot/jshint Code review by ezrobot
Details
ezrobot/phpcsfixer Code review by ezrobot
Details
ezrobot/yuidoc Code review by ezrobot
Details

@andrerom andrerom deleted the eztable branch Jun 13, 2017

@andrerom andrerom removed the Ready for QA label Jun 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.