Builder - Content Editor - Web page (C039 - #32) #4600

ghost · 2019-12-04T12:17:09Z

Description

In this PR I've addeed create new geostory component mechanism - WebPage

Issues

Builder - Content Editor - Web page #4514

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/geosolutions-it/MapStore2/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x", remove the others)

Feature

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x", remove the other)

No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

coveralls · 2019-12-04T12:33:40Z

Coverage decreased (-0.03%) to 84.444% when pulling d82fe49 on kboczkowski-clurgo:feature/C039-32 into 91cf087 on geosolutions-it:master.

MV88

Hi, there are some problems with this pr

@tdipisa I noticed that you cannot add directly a web page content, you have to add a media section or paragraph

Open a new story
Add a media section and select an image
Add a first web content with aaaa
Add a second web content with bbb, below the previous one
edit the first one with ccccc

current result

you cannot save, the modal does not closes, and it updates the wrong web content

expected

to save correct web content
I'm not able to reproduce the following error

Uncaught Invariant Violation: branch(Preview)(...): Nothing was returned from render. This usually means a return statement is missing. Or, to render nothing, return null.
    at eval (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:15693:36)
    at reconcileChildFibers (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:15696:15)
    at reconcileChildren (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:18026:28)
    at mountIndeterminateComponent (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:18794:5)
    at beginWork$1 (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:20084:16)
    at HTMLUnknownElement.callCallback (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:362:14)
    at Object.invokeGuardedCallbackDev (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:411:16)
    at invokeGuardedCallback (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:466:31)
    at beginWork$$1 (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:25730:7)
    at performUnitOfWork (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:24638:12)
    at workLoopSync (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:24614:22)
    at performSyncWorkOnRoot (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:24182:11)
    at eval (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:12238:24)
    at unstable_runWithPriority (webpack:///./node_modules/scheduler/cjs/scheduler.development.js?:815:12)
    at runWithPriority$2 (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:12188:10)
    at flushSyncCallbackQueueImpl (webpack:///./node_modules/react-dom/cjs/react-dom.development.js?:12233:7)

tdipisa · 2019-12-05T09:00:16Z

@kboczkowski-clurgo please fix the issues identified by @MV88, the Web Page content must be added not only as an additional content of an existing section (immersive or not) but also as a section per se like the other ones (images, paragraph etc). The requirement in the issue description is quite clear about that I think.

…ore2 into feature/C039-32

tdipisa · 2019-12-06T08:39:26Z

@tdipisa I noticed that you cannot add directly a web page content, you have to add a media section or paragraph

@kboczkowski-clurgo looking at the last commit message I suppose that latest updates don't fix this problem but only the consol error. Isn't it?

ghost · 2019-12-06T08:44:44Z

@tdipisa actualy I've fixed problem with editing which @MV88 discover. Now I'm working on adding this component via section directly.

MV88

Hi, I have found something that can be improved.

For the moment, use the same "code" icon for webpage content in section p
Please align the webpage section with other ones in a single row
can you avoid to close the modal by clicking on the shadow (similar to media editor)?
I have found a weird interaction with drag and drop of web page content
The drag and drop, works, but the modal appears
Add url validation, maybe via a regex. See if there is already something in the codebase, maybe among utils

ghost · 2019-12-09T12:06:24Z

#4600 (review)
It's done

MV88

Hi, i was not able to load these urls:

MV88 · 2019-12-09T13:41:07Z

web/client/components/geostory/builder/SectionsPreview.jsx

@@ -139,6 +140,7 @@ const previewContents = {
                    const contentType = content.type === 'column'
                        ? `${content.type}${content.align || 'center'}`
                        : content.type;
+                    console.log(contentType, content.type);


Suggested change

console.log(contentType, content.type);

MV88 · 2019-12-09T13:44:44Z

web/client/utils/URLUtils.js

+ * Validator of URL
+ * @param {string} url - url to validate
+ */
+export const isValidURL = (url) => {


Hello, can you add tests for this, to see which use cases we are considering

tdipisa · 2019-12-09T15:13:41Z

Hi, i was not able to load these urls:

http://localhost:8081/?debug=true#/geostory/newgeostory/

http://localhost:8081/#/geostory/newgeostory/

This doesn't make sens to me, why we should support these kind of URLs?

Maybe what we can do here is just to make the regex configurable from plugin config

@offtherailz what do you think?

offtherailz · 2019-12-09T15:35:04Z

the only problem concerns "localhost", because the regex denies domain names without a point. This may fail with host files or local DNS.

Because the system may need to satisfy different requirements, as @tdipisa suggested, I should allow to configure the regex (2nd param of isValidURL may be the regex, by default the one currently used, maybe without the . check, but this is not necessary, if configurable.
From outside, (where isValidURL is called) you can get it from ConfigUtils.getConfigProp("GeoStoryValidIframeURLRegex") and pass it as 2nd param. I think it's enough.

ghost · 2019-12-10T08:13:36Z

@MV88 @tdipisa @offtherailz PR is updated with changes suggested by @offtherailz. Now validation util has optional parameter to pass custom regexp.

MV88

Hi @kboczkowski-clurgo just a minor update needed

MV88 · 2019-12-10T08:38:34Z

web/client/components/geostory/contents/WebPageWrapper.jsx

+
+    save = () => {
+        const { url } = this.state;
+        const error = !isValidURL(url);


can you pass ConfigUtils.getConfigProp("GeoStoryValidIframeURLRegex") as second argument?
in that case we can override it anytime without changing the code

Verify that if the property is missing in config, then it uses the default one

MV88 · 2019-12-10T08:40:20Z

web/client/components/geostory/contents/WebPageWrapper.jsx

+    save = () => {
+        const { url } = this.state;
+        const error = !isValidURL(url);
+        this.setState({ error: !isValidURL(url) });


Suggested change

this.setState({ error: !isValidURL(url) });

this.setState({ error });

MV88

Hi, i think now we are good

C039 #32: added WebPage create mechanism in geostory

582f063

ghost added Priority: High GeoStory C039-2020-HALLIBURTON-SUPPORT labels Dec 4, 2019

ghost requested a review from MV88 December 4, 2019 12:17

ghost self-assigned this Dec 4, 2019

ghost mentioned this pull request Dec 4, 2019

Builder - Content Editor - Web page (C039 - #32) #4547

Closed

12 tasks

Merge branch 'master' into feature/C039-32

6ad6844

Merge branch 'master' into feature/C039-32

d3053fd

MV88 requested changes Dec 4, 2019

View reviewed changes

Krzysztof Boczkowski added 2 commits December 6, 2019 08:49

fixed error with edit geostory webpage component

4fc4f32

Merge branch 'feature/C039-32' of github.com:kboczkowski-clurgo/MapSt…

aafc98a

…ore2 into feature/C039-32

feature: adding web page as section

f94cb4a

ghost requested a review from MV88 December 6, 2019 13:26

MV88 requested changes Dec 9, 2019

View reviewed changes

added validation and minor fixes

146473d

ghost requested a review from MV88 December 9, 2019 12:06

code cleanup

faff1f0

MV88 requested changes Dec 9, 2019

View reviewed changes

updated url validation regexp and added tests

bf98915

ghost requested a review from MV88 December 10, 2019 08:13

MV88 requested changes Dec 10, 2019

View reviewed changes

added config support

d82fe49

ghost requested a review from MV88 December 10, 2019 09:04

MV88 approved these changes Dec 10, 2019

View reviewed changes

tdipisa merged commit 0424c9d into geosolutions-it:master Dec 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Builder - Content Editor - Web page (C039 - #32) #4600

Builder - Content Editor - Web page (C039 - #32) #4600

ghost commented Dec 4, 2019 •

edited by simboss

Loading

coveralls commented Dec 4, 2019 •

edited

Loading

MV88 left a comment

tdipisa commented Dec 5, 2019 •

edited

Loading

tdipisa commented Dec 6, 2019

ghost commented Dec 6, 2019

MV88 left a comment •

edited

Loading

ghost commented Dec 9, 2019

MV88 left a comment

MV88 Dec 9, 2019

MV88 Dec 9, 2019

tdipisa commented Dec 9, 2019

offtherailz commented Dec 9, 2019

ghost commented Dec 10, 2019

MV88 left a comment

MV88 Dec 10, 2019

ghost Dec 10, 2019

ghost Dec 10, 2019

MV88 Dec 10, 2019

MV88 left a comment

	this.setState({ error: !isValidURL(url) });
	this.setState({ error });

Builder - Content Editor - Web page (C039 - #32) #4600

Builder - Content Editor - Web page (C039 - #32) #4600

Conversation

ghost commented Dec 4, 2019 • edited by simboss Loading

Description

Issues

coveralls commented Dec 4, 2019 • edited Loading

MV88 left a comment

Choose a reason for hiding this comment

tdipisa commented Dec 5, 2019 • edited Loading

tdipisa commented Dec 6, 2019

ghost commented Dec 6, 2019

MV88 left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Dec 9, 2019

MV88 left a comment

Choose a reason for hiding this comment

MV88 Dec 9, 2019

Choose a reason for hiding this comment

MV88 Dec 9, 2019

Choose a reason for hiding this comment

tdipisa commented Dec 9, 2019

offtherailz commented Dec 9, 2019

ghost commented Dec 10, 2019

MV88 left a comment

Choose a reason for hiding this comment

MV88 Dec 10, 2019

Choose a reason for hiding this comment

ghost Dec 10, 2019

Choose a reason for hiding this comment

ghost Dec 10, 2019

Choose a reason for hiding this comment

MV88 Dec 10, 2019

Choose a reason for hiding this comment

MV88 left a comment

Choose a reason for hiding this comment

ghost commented Dec 4, 2019 •

edited by simboss

Loading

coveralls commented Dec 4, 2019 •

edited

Loading

tdipisa commented Dec 5, 2019 •

edited

Loading

MV88 left a comment •

edited

Loading