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

fix: remove overlay that is preventing unrelated UI interactions on the page #98

Merged
merged 2 commits into from
Jul 23, 2020

Conversation

markocikos
Copy link
Member

@carloslancha @mateomustapic @wincent

This is a followup on liferay-frontend/liferay-portal#145. Instead of overriding the overlay in portal, we are removing it from the source.

I am not sure what is the workflow when sending PR on this repo. I included the commit with generated build, is that correct to include? I am assuming that we need to publish to npm once this is merged. Last time I did this, but more a as a learning feat. Is it still ok for me to do it? There is also this open PR liferay-frontend/liferay-portal#149 where we update ckeditor version, so we need to be a bit careful of conflicts.

@markocikos markocikos changed the title LPS-117288 If I have more than one HTML field on a web content I'm not able to edit on click fix: remove overlay that is preventing unrelated UI interactions on the page Jul 22, 2020
@carloslancha
Copy link
Contributor

Hey @markocikos could you please add some screenshots of before/after? Did you check ckeditor and allloy are working/looking as expected in the rest of portal? I'm a little worried about removing the entire class 😅

@carloslancha
Copy link
Contributor

I included the commit with generated build, is that correct to include

I usually don't include it and just build it while preparing the release, but there should be no problem.

There is also this open PR liferay-frontend/liferay-portal#149 where we update ckeditor version, so we need to be a bit careful of conflicts.

There will be no conflicts, in that pr we're just updating version to .4. When we release .5 including this we'll send another pr to portal :)

@mateomustapic
Copy link
Contributor

I included the commit with generated build, is that correct to include

I usually don't include it and just build it while preparing the release, but there should be no problem.

There is also this open PR liferay-frontend/liferay-portal#149 where we update ckeditor version, so we need to be a bit careful of conflicts.

There will be no conflicts, in that pr we're just updating version to .4. When we release .5 including this we'll send another pr to portal :)

These changes will not fix the wrong background in page editor I removed in liferay-frontend/liferay-portal@c762472.

Also, if you @carloslancha agree, I can make a release of .5 version after this is done and merged.

@carloslancha
Copy link
Contributor

Hey @mateomustapic I'd first try to solve liferay-frontend/liferay-portal#148 in liferay-ckeditor so we can ship a new version with both issues fixed. If there's no fix we can make here for that issue then it's ok for me to release .5 once this is merged.

Anyway, we'll need to do the process together so we can find out what went wrong the last time (permissions to publish, some error during build...) and solve it for further releases 😄

@markocikos
Copy link
Member Author

These changes will not fix the wrong background in page editor I removed in liferay-frontend/liferay-portal@c762472.

@mateomustapic I am fairly certain they will, but I cannot reproduce steps in https://issues.liferay.com/browse/LPS-117468

How do you get to the point where you can edit text in hello world app? For me, nothing happens when I click on it.

@carloslancha
Copy link
Contributor

carloslancha commented Jul 22, 2020

@markocikos double click on the edit view

@mateomustapic
Copy link
Contributor

These changes will not fix the wrong background in page editor I removed in liferay-frontend/liferay-portal@c762472.

@mateomustapic I am fairly certain they will, but I cannot reproduce steps in https://issues.liferay.com/browse/LPS-117468

How do you get to the point where you can edit text in hello world app? For me, nothing happens when I click on it.

On the Home page select Edit at the top and then double-click on "Hello world" text > Edit again

@markocikos
Copy link
Member Author

This is master, today's ant all, no errors in console. I do have profile-dxp, that may cause this. There is no Hello World text. Do I need to turn on headers or subtitles or something similar?

hello

@carloslancha
Copy link
Contributor

carloslancha commented Jul 22, 2020

Today's master with profile-dxp for me. Weird...

image

@mateomustapic
Copy link
Contributor

image

@markocikos
Copy link
Member Author

Well, whatever the reason my hello world app is going crazy, it is unrelated to this PR. @mateomustapic you can check that either this PR or liferay-frontend/liferay-portal#145 fixes the issue in your ticket.

@carloslancha
Copy link
Contributor

Hey @markocikos could you please add some screenshots of before/after? Did you check ckeditor and allloy are working/looking as expected in the rest of portal? I'm a little worried about removing the entire class 😅

please 🙏

@markocikos
Copy link
Member Author

markocikos commented Jul 22, 2020

an example of ckeditor in message boards and alloyeditor in web content:

Before overlay fix:

before-overlay-fix

After overlay fix:

after-overlay-fix

@carloslancha
Copy link
Contributor

LGTM then

@mateomustapic
Copy link
Contributor

Hey @mateomustapic I'd first try to solve liferay-frontend/liferay-portal#148 in liferay-ckeditor so we can ship a new version with both issues fixed. If there's no fix we can make here for that issue then it's ok for me to release .5 once this is merged.

Anyway, we'll need to do the process together so we can find out what went wrong the last time (permissions to publish, some error during build...) and solve it for further releases 😄

I found a way to solve this in liferay-ckeditor and I will prepare a PR soon.

@carloslancha
Copy link
Contributor

@mateomustapic nice! We'll wait for both prs to be merge to ship a new release then 🎉

@markocikos
Copy link
Member Author

Did you check ckeditor and allloy are working/looking as expected in the rest of portal? I'm a little worried about removing the entire class

I did not check all instances, that would take days if not weeks 🙂 But this is a reasonably safe change, I cannot think of a specific case when such an overlay would be useful. It would make sense if the whole implementation is built around it, but that would be noticed in every single editor.

@carloslancha
Copy link
Contributor

did not check all instances, that would take days if not weeks 🙂 But this is a reasonably safe change, I cannot think of a specific case when such an overlay would be useful. It would make sense if the whole implementation is built around it, but that would be noticed in every single editor.

Yep! I was not expecting you to check all instances, just be reasonably sure that this will not break other stuff 😄

@carloslancha carloslancha merged commit 6344d7f into liferay:master Jul 23, 2020
@carloslancha
Copy link
Contributor

Let's see what happens with #99 and decide if we ship a new version today or we wait for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants