Feature/react #946

julien · 2018-12-19T13:10:09Z

Please employ to following template as the base structure of Pull Requests for AlloyEditor (liferay/alloy-editor) project. Pull Requests not incorporating the sections below will be regarded as unsatisfying and rejected.

Prelude

Preliminary history of the issue related to this very Pull Request. Share any information that could ease the understanding of why this issue exists and what measures have been administered to address it.

Issue

Link the issue from the issues list with any complementary explanation that is required to have a solid understanding of it.

Solution

Line up the major changes that have been applied.

Test

Append a comprehensive list of devices against which the implementation was tested. This is relevant only if the issue described incorporates various behaviours on different devices.

If possible attach an animated GIF of the major scenarios that have been affected by the change, so that non-techincal participants can also have a better undertanding of the subject matter.

Fix #845 - Remove resizer everytime the cursor gets out of the pillar. Keeping the resizer there without any reason creates a black hole where the CKEditor's mousemove event is never triggered

…browsers

Fix autolink test

Fix #898 | Added center align css interaction between IE11 and other …

LPS-87500 Add styleFn support for the ButtonStylesList component

Fixes #911 Use correct CSS to place drag resize handle at the corner

Fixes #900 Add closing …

Fixes #915 - Checks if selection data exists in linkSelectionTest

…de the style attribute so as to be consistent with the standard AlloyEditor resizing behavior.

Change dragresize implementation on IE to modify heigh and width insi…

Fixes #928 | Update gulp-sass to work with node v10.x.x

Fixes #931 | Fixes the saucelabs browser-matrix badge

…react

…nment

…react

julien · 2018-12-19T13:41:20Z

Hey @bryceosterhaus and @matuzalemsteles,

This is an attempt to update react to the latest version (at this time v16.6.3) and apply various fixes we made.

If you have a moment could you please take a look.

There are still things to be done, for example:

Some files are still written in ES5 (inside a IIFE )
Some tasks still need to be "fine tuned", or missing (no format or lint tasks)

But I think that the most important thing is checking that everything works fine (which I did but I think a second look will be more than welcome) so that we can continue "pushing alloy-editor forward"

Thanks for your help and time!

bryceosterhaus

Hey @julien,

I noticed a few things, nothing major but just wanted to get your thoughts on it.

I also noticed a bunch of whitespace changes, I assume this is just because we don't have a lint or format task? Also, any idea why we have a range of file extensions? I noticed we had .js for both es5 and es6 code and then also had some .jsx files mixed in there too.

Other than those few things, it looks good so far, glad to see we are updating it.

Also, I only looked at the update commit, should've I looked at the whole PR?

bryceosterhaus · 2018-12-19T20:10:44Z

src/components/base/button-style.js

+};
+
+// WrappedComponent.propTypes = {


Is this block supposed to be commented out?

Well, no ... but I doesn't seem to be needed either.
I'll remove it and clean that.

bryceosterhaus · 2018-12-19T20:12:50Z

src/components/base/toolbar-buttons.js

@@ -45,10 +72,10 @@ export default WrappedComponent => class extends WrappedComponent {
                    return button;
                })
            )
-            .map(function(button) {
+            .map(function(button, index) {
                var props = this.mergeExclusiveProps({
                    editor: this.props.editor,


Is this working properly here? I would expect it to be referring to the function(button, index) closure.

bryceosterhaus · 2018-12-19T20:14:01Z

src/components/base/toolbar-buttons.js

+     * @return {Array} An Array which contains the buttons or button groups that should be rendered.
+     */
+    getToolbarButtonGroups(buttons, additionalProps) {
+        var instance = this;


I don't remember what our pattern for this is in portal, but it seems weird to be setting var instance = this; for a react component. Will we ever be using instance and this where they reference to two different things?

I agree that it seems weird in a React.Component but for the moment I'm just copying the existing code over ...

A possible improvement could simply be using an arrow function

return buttons.reduce((list, button) => { if (Array.isArray(button)) { list.push(this.getToolbarButtons(button, additionalProps)); return list; } else { return this.getToolbarButtons(buttons, additionalProps); } }, []);

Oh gotcha, I wasn't aware it was just copied over. Should be fine as is, we can update and refactor later.

bryceosterhaus · 2018-12-19T20:15:19Z

src/components/buttons/button-paragraph-align.jsx

+		var buttonCommandsListId;
+
+		if (this.props.expanded) {
+				buttonCommandsListId = ButtonParagraphAlign.key + 'List';


whitespace looks a bit too large here. Maybe its just GH's rendering of it though.

Yes, that's actually because the project has "mixed spaces and tabs" ...
I'll reformat it for now.
In this project the .eslintrc file specifices we should be using 4 spaces for indentation.

We might want to change that later when we add a format/lint task and use tabs like in other projects?

Yeah using tabs is probably best. At least to just be consistent across projects.

Great I'll add that when I work on #926

bryceosterhaus · 2018-12-19T20:18:40Z

src/components/buttons/button-separator.jsx

+ *
+ * @class ButtonSeparator
+ */
+class ButtonSeparator extends React.Component {


Would it make sense to use a function component here since there is no internal state?

I'm not positive about the implications of it with the latest react version, but I remember there being performance implications in the past. Using function components were preferred over class components if there was no state.

Ok, I'll check that out, sounds like a good idea.

@bryceosterhaus Ok so I changed this to a function component, but it still makes has a static key property, tell me if that is an issue.

That should be fine!

bryceosterhaus · 2018-12-19T20:20:20Z

src/components/toolbars/toolbar-styles.jsx

-                currentSelection.buttons,
+            var buttons = currentSelection.buttons;
+
+            if (typeof buttons === 'object' && !Array.isArray(buttons)) {


Do we have any helpers available that we could use for these sorts of things? idk if we want to include metal-core for some of those things.

Personally, I don't really know either.
It might be an improvement for later, but for the moment I don't think it's much of a problem having this like it is now.

src/components/toolbars/toolbar-styles.jsx

bryceosterhaus · 2018-12-19T20:22:03Z

src/components/toolbars/toolbar-styles.jsx

+                            buttonsGroup.map(function (value, index) {
+                                if (Array.isArray(value)) {
+                                    return (
+                                        <div className="ae-row" key={index.toString()}>


Is the toString() is necessary here? Seems like you could just pass the index to key and it would be fine.

You're right, it's not needed

bryceosterhaus · 2018-12-19T20:22:36Z

src/components/toolbars/toolbar-styles.jsx

+                                        <div className="ae-row" key={index.toString()}>
+                                            {
+                                                value.map(function (button) {
+                                                    return button;


You might also want to add a key here somehow for safe keeping.

You mean in the button?

You actually might be fine since you aren't returning a jsx element. Usually react will complain about not having a key attr on lists. https://reactjs.org/docs/lists-and-keys.html

Just so you know this button is actually an element, but be reassured it has a key

As long as there are no client-side errors for the "key", it should be good! React is usually pretty good at emitting warnings to the console for things like this.

julien · 2018-12-20T11:30:19Z

Hey @bryceosterhaus,

Thanks for the review, yes the idea was to review the last commit.

bryceosterhaus · 2018-12-20T21:17:22Z

@julien Other than the few changes you noted in making, it looks good to me. Any code that you copied over, I wouldn't worry about changing or refactoring unless its needed by the version change.

bryceosterhaus · 2018-12-21T17:47:20Z

Updated changes look good to me!

julien · 2018-12-26T13:36:15Z

@bryceosterhaus thanks again for the review!

julien and others added 30 commits October 23, 2018 10:01

Merge pull request #893 from liferay/pr-892

91238c5

Fix #845 - Remove resizer everytime the cursor gets out of the pillar. Keeping the resizer there without any reason creates a black hole where the CKEditor's mousemove event is never triggered

1.5.15

6189e19

Build Files (auto-generated)

885782f

Updates CHANGELOG for v1.5.15

a196faf

Fix #898 | Added center align css interaction between IE11 and other …

96af150

…browsers

Update saucelabs configuration

0afdb5e

Try to fix autolink test

1ab8b95

Merge pull request #907 from julien/test_autolink

d97896d

Fix autolink test

Merge pull request #899 from fortunatomaldonado/Fix#898

18ac73d

Fix #898 | Added center align css interaction between IE11 and other …

Fixes #911 Use correct CSS to place drag resize handle at the corner

61380a1

Fixes #900 Add closing semi-colon in IE11

fde5fff

LPS-87500 Add styleFn support for the ButtonStylesList component

192a163

Merge pull request #916 from pavel-savinov/LPS-87500

89e3fcd

LPS-87500 Add styleFn support for the ButtonStylesList component

Merge pull request #912 from brianikim/LPS-87458

aab69a8

Fixes #911 Use correct CSS to place drag resize handle at the corner

Merge pull request #913 from SpencerWoo/issue900

f7a168b

Fixes #900 Add closing …

Fixes #915 - Checks if selection data exists in linkSelectionTest

5397859

Merge pull request #918 from liferay/pr-917

f88abdc

Fixes #915 - Checks if selection data exists in linkSelectionTest

Fixes #908 Remove unwanted tags after paste

de331d4

1.5.16

1d404b0

Build Files (auto-generated)

7ea9e76

Updates CHANGELOG for v1.5.16

8b3925b

Change dragresize implementation on IE to modify heigh and width insi…

ff4b6da

…de the style attribute so as to be consistent with the standard AlloyEditor resizing behavior.

Merge pull request #923 from gregory-bretall/issue-901-updated

7f87818

Change dragresize implementation on IE to modify heigh and width insi…

Fixes #928 | Update gulp-sass to work with node v10.x.x

abed0b1

Merge pull request #929 from liferay/feature/node_v10.x

173bb54

Fixes #928 | Update gulp-sass to work with node v10.x.x

Fixes #931 | Fixes the saucelabs browser-matrix badge

d8f3fc0

Fixes #829 | Limit tweet text to 280 chars

0139136

Merge pull request #932 from liferay/fixes/readme

6138c8b

Fixes #931 | Fixes the saucelabs browser-matrix badge

Fixes #924 Add styling for four drag handles of image resizer

0f83987

Fixes #924 Add four drag handles to image resizer

7406081

Julien Castelain and others added 19 commits December 17, 2018 11:50

Fixes #931 | Fixes the saucelabs browser-matrix badge

2fb1040

Fix conflict for 0139136

0fbffa9

Merge commit '6138c8b37baac5b652c36e35b04525a99b5136dc' into feature/…

de5e755

…react

Fixes #924 Add styling for four drag handles of image resizer

85f2f1c

Fixes #924 Add four drag handles to image resizer

d6145d2

Fixes #924 No longer need to add/remove CSS class based on image alig…

dbd174b

…nment

Merge commit '73f472942d0e14e41ecdab12cccb902fb34cf7db' into feature/…

5ec74cf

…react

Fixes #853 | Updates toolbar position when changing image alignment

895aee0

Merge commit '5d9b4805fa2dcf1273ef960577c1cb3fcfd6b691' into feature/…

f96c8b4

…react

Merge commit '2949f3e475793483e80bed3c1ee80087a6cd71d8' into feature/…

aa7c755

…react

Update ckeditor

7781c79

Fix conflict for 1ab0230

5f7c042

Fixes #906 | Reorder toolbar buttons for full mode

da89ed5

Fix conflict for c95d8dc

82f4f06

Merge commit '1076f991747dc4a62cfada84b127be1d70610616' into feature/…

35fca37

…react

Fix conflict for 5aabd16

2b835f4

Fix conflict for ffe6112

2377184

Updates CHANGELOG for v1.5.17

73db55c

Update to React v16.6.3

a257367

bryceosterhaus requested changes Dec 19, 2018

View reviewed changes

Julien Castelain and others added 3 commits December 21, 2018 11:49

Apply requested changes

1577fb4

Merge branch '2.x-develop' into feature/react

861ed73

Regen package-lock

bf95697

julien merged commit 5da5cde into liferay:2.x-develop Dec 26, 2018

julien deleted the feature/react branch February 14, 2019 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/react #946

Feature/react #946

julien commented Dec 19, 2018

julien commented Dec 19, 2018

bryceosterhaus left a comment •

edited

Loading

bryceosterhaus Dec 19, 2018

julien Dec 20, 2018

bryceosterhaus Dec 19, 2018

julien Dec 20, 2018

bryceosterhaus Dec 19, 2018

julien Dec 20, 2018

bryceosterhaus Dec 20, 2018

julien Dec 21, 2018

bryceosterhaus Dec 19, 2018

julien Dec 20, 2018

bryceosterhaus Dec 20, 2018

julien Dec 21, 2018

bryceosterhaus Dec 19, 2018

julien Dec 20, 2018

julien Dec 21, 2018

bryceosterhaus Dec 21, 2018

bryceosterhaus Dec 19, 2018

julien Dec 20, 2018

bryceosterhaus Dec 19, 2018

julien Dec 20, 2018

bryceosterhaus Dec 19, 2018

julien Dec 20, 2018

bryceosterhaus Dec 20, 2018

julien Dec 21, 2018

bryceosterhaus Dec 21, 2018

julien commented Dec 20, 2018

bryceosterhaus commented Dec 20, 2018

bryceosterhaus commented Dec 21, 2018

julien commented Dec 26, 2018

Feature/react #946

Feature/react #946

Conversation

julien commented Dec 19, 2018

Prelude

Issue

Solution

Test

julien commented Dec 19, 2018

bryceosterhaus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien commented Dec 20, 2018

bryceosterhaus commented Dec 20, 2018

bryceosterhaus commented Dec 21, 2018

julien commented Dec 26, 2018

bryceosterhaus left a comment •

edited

Loading