EZP-28841: Content On the Fly v2 #55

dew326 · 2018-03-02T08:15:25Z

Jira ticket: https://jira.ez.no/browse/EZP-28841
Requires: ezsystems/ezplatform-admin-ui#366

TODO

content on the fly in UDW
forced language
check permission - missing endpoint

sunpietro

There's quite a lof of things to change.

sunpietro · 2018-03-07T07:54:10Z

src/modules/universal-discovery/components/content-creator/content.creator.component.js

+import './css/content.creator.component.css';
+
+export default class ContentCreatorComponent extends Component {
+    constructor(props) {


If you're not using props to initialize state, then you can omit the props variable in the constructor method.

You can but I don't think you should.
Class components should always call the base constructor with props. from React doc: https://reactjs.org/docs/state-and-lifecycle.html#adding-local-state-to-a-class

This is recommendation, right. We can follow this rule. It's especially useful when you're extending your own component classes. In this case, when you extend base component class it's not that relevant. Because any kind of props won't affect the way the base class works.

I know it will not cause any problems when extending React.Component for now, but if they have this in their doc they may require this at some point in the future.

Ok, let's keep it then. From the currect perspective, it brings no value now, but it's a recommended by React doc.

sunpietro · 2018-03-07T07:57:58Z

src/modules/universal-discovery/components/content-creator/content.creator.component.js

+    handleIframeLoad() {
+        const locationId = this._iframe.contentWindow.document.querySelector('meta[name="LocationID"]');
+
+        if (this._iframe.contentWindow.location.pathname !== this.generateIframeUrl() && !locationId) {


Can this.generateIframeUrl() be cached?

sunpietro · 2018-03-07T08:00:11Z

src/modules/universal-discovery/components/content-creator/content.creator.component.js

+        } else {
+            this.setState(state => Object.assign({}, state, { iframeLoading: false }));
+
+            this._iframe.contentWindow.onbeforeunload = () => {


Question: I would put the iframe event handlers to the second callback of this.setState method. Do you think it's a bad approach? When moving the event handlers assignment to the callback you apply them right the state is updated (in a sync way).

The question is still valid.

Those onbeforeunload and onunload is not related to the setState, I think callback for setState should be used when its depends on the state.

sunpietro · 2018-03-07T08:08:42Z

src/modules/universal-discovery/components/content-creator/content.creator.component.js

+    }
+
+    generateIframeUrl() {
+        return window.Routing.generate('ezplatform.content.create.on_the_fly', {


I think it should be initialized in the constructor method, because you're not updating app state in case of props updates. This function will always return the same output.
In the constructor it should be stored liked this:

this.iframeUrl = window.Routing.generate(...);

sunpietro · 2018-03-07T08:09:02Z

src/modules/universal-discovery/components/content-creator/content.creator.component.js

+     * @returns {Element}
+     * @memberof FinderTreeLeafComponent
+     */
+    renderLoadingIcon() {


renderLoadingSpinner

sunpietro · 2018-03-07T10:54:33Z

src/modules/universal-discovery/components/tab-content/create.panel.component.js

+        <div {...wrapperAttrs}>
+            <TabContentPanelComponent {...props}>
+                <div className="c-create-panel__first-step">
+                    <div className="c-create-panel__step-title">1) {props.labels.contentOnTheFly.chooseLangaugeAndContentType}</div>


Using ol > li isn't the right way to go?

So li would need to have inside a whole create or finder component? I think it's not the best structure.

sunpietro · 2018-03-07T10:57:12Z

src/modules/universal-discovery/services/universal.discovery.service.js

+        languageCode,
+        locationId
+    }, callback) => {
+    const endpoint = window.Routing.generate('ezplatform.content.create.on_the_fly.has_access', {


There's no need to duplicate the names: window.Routing.generate('ezplatform.content.create.on_the_fly.has_access', { languageCode, contentTypeIdentifier, locationId })

sunpietro · 2018-03-07T10:58:07Z

src/modules/universal-discovery/services/universal.discovery.service.js

+    const request = new Request(endpoint, {
+        method: 'GET',
+        headers: {'X-CSRF-Token': token},
+        mode: 'cors',


We decided to switch to proper mode: same-origin

sunpietro · 2018-03-07T11:00:32Z

src/modules/universal-discovery/universal.discovery.module.js

+        this.onContentTypeSelected = this.onContentTypeSelected.bind(this);
+        this.handlePublish = this.handlePublish.bind(this);
+
+        if (props.forcedLanguageCOTF) {


I believe it should be renamed to something like: cotfForcedLanguage or maybe it can be done differently. Because, here youre hardcoding a kind-of-dependency to the COTF which might not exist on a specific instalation.
Do you really need this information here, in UDW module?

COTF is part of a UDW module and the configuration must be set on the UDW and then pass to the COTF.
Because, here youre hardcoding a kind-of-dependency to the COTF which might not exist on a specific instalation. - that's why it has a default value '' which will prevent from breaking in customers instalations

sunpietro · 2018-03-07T11:01:42Z

src/modules/universal-discovery/universal.discovery.module.js

+            this.setState(state => Object.assign({}, state, {isCheckingPermission: false}));
+
+            if (this.state.hasPermission !== response.access) {
+               this.setState(state => Object.assign({}, state, {hasPermission: response.access}));


I think you should set state once, not twice in the same scope.

I'm not sure how this could be in one. We want to change isCheckingPermission every time when the request is finished but change the permission state only when have to.

What do you think about it?

this.setState(state => Object.assign({}, state, { isCheckingPermission: false, hasPermission: state.hasPermission !== response.access ? response.access : state.hasPermission }));

I can try but as far as I know thanks to immutable with Object.assign this will make rerender of react component and this is what I wanted to avoid in this situation.

You're already doing it, so why doing it twice?

React will compare boolean values (I assume they are boolean). If there's any change then it will cause re-rendering of any related piece of code.

dew326 · 2018-03-12T10:33:17Z

ping @sunpietro

sunpietro · 2018-03-12T11:41:48Z

src/modules/universal-discovery/components/content-creator/content.creator.component.js

+    }
+
+    handlePublish() {
+        this._iframe.contentWindow.onbeforeunload = () => {};


With ReactJS we abandoned the rule of adding underscores into protected variables. These will not be exposed in the React code anyway.

sunpietro · 2018-03-12T11:43:11Z

src/modules/universal-discovery/components/content-creator/content.creator.component.js

+        } else {
+            this.setState(state => Object.assign({}, state, { iframeLoading: false }));
+
+            this._iframe.contentWindow.onbeforeunload = () => {


The question is still valid.

sunpietro · 2018-03-12T11:44:42Z

src/modules/universal-discovery/components/content-creator/content.creator.component.js

+                .replace('{contentType}', selectedContentType.name)
+                .replace('{language}', selectedLanguage.name);
+        const iframeUrl = this.generateIframeUrl();
+        const contentClass = this.state.iframeLoading ? "m-ud__content is-loading" : "m-ud__content";


CS: Single quotes, please.

Why not: m-ud__content--is-loading?

sunpietro · 2018-03-12T11:47:03Z

src/modules/universal-discovery/components/create/create.choose.content.type.component.js

+    }
+
+    updateFilterQuery(event) {
+        const value = event.target.value.toLowerCase();


Have you thought about naming it as filterQuery?

sunpietro · 2018-03-12T11:48:02Z

src/modules/universal-discovery/components/create/create.choose.content.type.component.js

+        }, 200);
+    }
+
+    renderItem(item, index) {


The question is still valid.

sunpietro · 2018-03-12T11:51:37Z

src/modules/universal-discovery/components/create/css/create.choose.content.type.component.css

+    background-color: #e1f5ff;
+}
+
+


CS: Remove extra empty line here.

sunpietro · 2018-03-12T11:53:04Z

src/modules/universal-discovery/components/tab-content/create.panel.component.js

+        <div {...wrapperAttrs}>
+            <TabContentPanelComponent {...props}>
+                <div className="c-create-panel__first-step">
+                    <div className="c-create-panel__step-title">1) {props.labels.contentOnTheFly.chooseLangaugeAndContentType}</div>


sunpietro · 2018-03-12T11:53:52Z

src/modules/universal-discovery/services/universal.discovery.service.js

+
+    fetch(request)
+        .then(handleRequestResponse)
+        .then(json => callback(json))


.then(callback)

sunpietro · 2018-03-12T11:54:22Z

src/modules/universal-discovery/services/universal.discovery.service.js

+
+    fetch(request)
+        .then(handleRequestResponse)
+        .then(json => callback(json))


.then(callback)

sunpietro · 2018-03-12T12:29:37Z

src/modules/universal-discovery/universal.discovery.module.js

+        if (this.props.onlyContentOnTheFly) {
+            return (
+                <div className="m-ud__panels">
+                    {this.renderSinglePanel(createPanelConfig)}


I'm thinking about moving these configs into some kidn of array. But maybe in other PR.

sunpietro · 2018-03-12T13:22:09Z

src/modules/universal-discovery/components/create/create.choose.content.type.component.js


        window.clearTimeout(this._filterTimeout);

        this._filterTimeout = window.setTimeout(() => {
-            this.setState(state => Object.assign({}, state, {filterQuery: value}));
+            this.setState(state => Object.assign({}, state, {filterQuery: filterQuery}));


{ filterQuery }

sunpietro · 2018-03-12T13:22:30Z

src/modules/universal-discovery/components/create/create.choose.language.component.js

@@ -21,14 +21,14 @@ export default class ChooseLanguageComponent extends Component {

        this.props.onLanguageSelected(selectedLanguage);

-        this.setState(state => Object.assign({}, state, {selected: selectedLanguage}));
+        this.setState(state => Object.assign({}, state, {selectedLanguage: selectedLanguage}));


{ selectedLanguage }

sunpietro · 2018-03-12T13:24:10Z

src/modules/universal-discovery/components/create/create.component.js

-            </div>
-        );
-    }
+const CreateComponent = (props) => {


You can drop import of React component in the first line: import React from 'react';

dew326 mentioned this pull request Mar 2, 2018

EZP-28841: Content On the Fly v2 ezsystems/ezplatform-admin-ui#366

Merged

6 tasks

dew326 self-assigned this Mar 2, 2018

dew326 added the Improvement label Mar 2, 2018

dew326 force-pushed the cotf branch from 34b0b46 to 1893fbb Compare March 6, 2018 14:22

dew326 changed the title ~~[WIP] EZP-28841: Content On the Fly v2~~ EZP-28841: Content On the Fly v2 Mar 7, 2018

dew326 requested a review from sunpietro March 7, 2018 07:50

dew326 force-pushed the cotf branch from 1893fbb to 4adddda Compare March 7, 2018 07:54

sunpietro suggested changes Mar 7, 2018

View reviewed changes

sunpietro suggested changes Mar 12, 2018

View reviewed changes

sunpietro reviewed Mar 12, 2018

View reviewed changes

sunpietro approved these changes Mar 12, 2018

View reviewed changes

EZP-28841: Content On the Fly v2

ca5a9fe

dew326 force-pushed the cotf branch from 34dc21d to ca5a9fe Compare March 12, 2018 13:45

lserwatka merged commit b99d7e1 into ezsystems:master Mar 12, 2018

EZP-28841: Content On the Fly v2 #55

EZP-28841: Content On the Fly v2 #55

Conversation

dew326 commented Mar 2, 2018 • edited Loading

sunpietro left a 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

dew326 commented Mar 12, 2018

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

dew326 commented Mar 2, 2018 •

edited

Loading