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

LPS-125413 Use new ImageSelector component in image-selector tag and remove old one #1494

Closed
wants to merge 8 commits into from
Closed

LPS-125413 Use new ImageSelector component in image-selector tag and remove old one #1494

wants to merge 8 commits into from

Conversation

carloslancha
Copy link

@carloslancha carloslancha commented Feb 11, 2021

Following up on #1484

https://issues.liferay.com/browse/LPS-125413

In this pr we're removing the old image-selector AUI component in favor of the new React one.

We're adding some missing markup in the React component and removing no needed one for SSR in the JSP

Made also some refactor and renaming in the React Components.

Deprecate draggableImage in favor of new imageCropDirection attribute in ImageSelector tag to improve naming.

imageselector

Test Plan 1:

  • Go to Content & Data / Blogs
  • Create a new Blog
  • Select an image bigger than the container
  • Click and move vertically the picture to crop it in the desired position
  • Publish
  • Edit the blog
  • Check that the image has been cropped correctly

Test Plan 2:

  • Go to Content & Data / Blogs
  • Edit an existing Blog with an image
  • Check that you can't move the image since it's already cropped
  • Remove the image and repeat Test plan 1 from point 3.

Carlos Lancha added 4 commits February 11, 2021 13:54
…ttribute in favor of imageCropDirection. Add isDraggable logic to Tag.
…and using new ImageSelector react component
…name some methods to improve naming consistency. Add missing markup. Update props to fit new API from tag.
@liferay-continuous-integration
Copy link
Collaborator

To conserve resources, the PR Tester does not automatically run for every pull.

If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed.

If your pull was never tested, comment "ci:test" to run the PR Tester for this pull.

@carloslancha carloslancha changed the title LPS-125413 Get rid of useCallback usages since they're not needed. Rename some methods to improve naming consistency. Add missing markup. Update props to fit new API from tag. LPS-125413 Use new ImageSelector component in image-selector tag and remove old one Feb 11, 2021
@carloslancha
Copy link
Author

ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 9272e31bc0297f9ad114de2c3a67f68aec8ea3cd

Sender Branch:

Branch Name: LPS-125413-migrate-image-selector.2
Branch GIT ID: c3ff4808aa42117f064ee59e69d2f7eedcabf145

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@carloslancha
Copy link
Author

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

@boton
Copy link
Collaborator

boton commented Feb 11, 2021

Just started reviewing :)

:octocat: Sent from GH.

@boton
Copy link
Collaborator

boton commented Feb 11, 2021

I think the failure is not from this PR but it had not been detected until now, when a draft is autosaved we find the input coverImageFileEntryCropRegion and on blogs.js we are trying to retrieve the value with getElementById https://github.com/liferay/liferay-portal/blob/5860529bcad88ac3c9b2f76f7e5cd7c877e5cc23/modules/apps/blogs/blogs-web/src/main/resources/META-INF/resources/blogs/js/blogs.js#L363-L365 so it would be fine to add that missing id with the same value of the name.
https://github.com/liferay-lima/liferay-portal/blob/c3ff4808aa42117f064ee59e69d2f7eedcabf145/modules/apps/item-selector/item-selector-taglib/src/main/resources/META-INF/resources/image_selector/js/ImageSelector.js#L316

image

It is a little trickier to see because that part of the code is only passed when drafts are saved in the background, you only have to leave it open for a few seconds with some type of content to see the failure.

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

❌ ci:test:relevant - 18 out of 23 jobs passed in 2 hours 28 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 9272e31bc0297f9ad114de2c3a67f68aec8ea3cd

Upstream Comparison:

Branch GIT ID: 9272e31bc0297f9ad114de2c3a67f68aec8ea3cd
Jenkins Build URL: Acceptance Upstream DXP (master) #1542

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 18 out of 23 jobs PASSED
18 Successful Jobs:
For more details click here.

Failures unique to this pull:


Failures in common with acceptance upstream results at 9272e31:
  1. test-portal-acceptance-pullrequest-batch(master)/modules-semantic-versioning-jdk8/0
    Job Results:

    0 Tests Passed.
    1 Test Failed.

    1. AXIS_VARIABLE=0,label_exp=!master #378779

      Please fix semantic versioning on carloslancha/LPS-125413-migrate-image-selector.2

           [exec] Compiling JSP files of project ':apps:item-selector:item-selector-upload-web' with /opt/dev/projects/github/liferay-portal/bundles/tomcat-9.0.40/webapps/ROOT/WEB-INF/lib/util-taglib.jar as dependency in place of 'com.liferay.portal:com.liferay.util.taglib'
           [exec] 
           [exec] > Task :apps:item-selector:item-selector-upload-web:compileJSP
           [exec] 
           [exec] > Task :apps:item-selector:item-selector-web:generateJSPJava
           [exec] Compiling JSP files of project ':apps:item-selector:item-selector-web' with /opt/dev/projects/github/liferay-portal/bundles/tomcat-9.0.40/webapps/ROOT/WEB-INF/lib/util-taglib.jar as dependency in place of 'com.liferay.portal:com.liferay.util.taglib'
           [exec] Note: /opt/dev/projects/github/liferay-portal/modules/apps/item-selector/item-selector-web/build/jspc/org/apache/jsp/view_005fitem_005fselector_005fview_005fdescriptor_jsp.java uses unchecked or unsafe operations.
           [exec] Note: Recompile with -Xlint:unchecked for details.
           [exec] 
           [exec] > Task :apps:item-selector:item-selector-web:compileJSP
           [exec] 
           [exec] > Task :apps:item-selector:item-selector-upload-web:jar
           [exec] > Task :apps:item-selector:item-selector-upload-web:autoUpdateXml SKIPPED
           [exec] > Task :apps:item-selector:item-selector-upload-web:baseline SKIPPED
           [exec] > Task :apps:item-selector:item-selector-upload-web:syncVersions
           [exec] 
           [exec] > Task :apps:item-selector:item-selector-web:jar
           [exec] > Task :apps:item-selector:item-selector-web:autoUpdateXml SKIPPED
           [exec] > Task :apps:item-selector:item-selector-web:baseline SKIPPED
           [exec] > Task :apps:item-selector:item-selector-web:syncVersions
           [exec] Gradle build finished at 2021-02-11 15:44:29.158.
           [exec] 
           [exec] FAILURE: Build failed with an exception.
           [exec] 
           [exec] * What went wrong:
           [exec] Execution failed for task ':apps:item-selector:item-selector-taglib:baseline'.
           [exec] > A failure occurred while executing com.liferay.gradle.plugins.baseline.internal.work.BaselineWorkAction
           [exec]    > org.gradle.api.GradleException: Semantic versioning is incorrect while checking /opt/dev/projects/github/liferay-portal/tools/sdk/dist/com.liferay.item.selector.taglib-5.0.0.jar against /opt/dev/projects/github/liferay-portal/.gradle/caches/modules-2/files-2.1/com.liferay/com.liferay.item.selector.taglib/4.1.33/a3bf5bf4096100c1ee471d70507e6c555e596a81/com.liferay.item.selector.taglib-4.1.33.jar

@carloslancha
Copy link
Author

Thx @boton yup, it looks like some ids were missing there since the creation of the component. Just added'em and pushed the commit ;)

@carloslancha
Copy link
Author

ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

❌ ci:test:relevant - 21 out of 23 jobs passed in 2 hours 15 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 7985dd55f78d0b4e4960d1b8b07158787e32d29f

Upstream Comparison:

Branch GIT ID: 7985dd55f78d0b4e4960d1b8b07158787e32d29f
Jenkins Build URL: Acceptance Upstream DXP (master) #1549

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 21 out of 23 jobs PASSED
21 Successful Jobs:
For more details click here.

Failures unique to this pull:

For upstream results, click here.

@liferay-continuous-integration
Copy link
Collaborator

@nikki-pru
Copy link

nikki-pru commented Feb 16, 2021

Hi @carloslancha, it looks like the changes to resources/image_selector/page.jsp, specifically around <div class="image-wrapper> could be causing the failure. If the change is intentional, the test might need a fix. I'll pull your branch now and send you the fix.

Sent the fix here - https://github.com/carloslancha/liferay-portal/pull/280

@carloslancha
Copy link
Author

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

✔️ ci:test:relevant - 23 out of 23 jobs passed in 1 hour 20 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 14013e30ddc196a32bec7e7d3ad31bc64eff58be

Upstream Comparison:

Branch GIT ID: 3a1328763cf4d2349d46f0a0674429df321a469f
Jenkins Build URL: Acceptance Upstream DXP (master) #1560

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 23 out of 23 jobs PASSED
23 Successful Jobs:
For more details click here.

@carloslancha
Copy link
Author

ci:forward

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:relevant
  •     ci:test:sf

The pull request will automatically be forwarded to the user brianchandotcom if the following test suites pass:

  •     ci:test:relevant
  •     ci:test:sf
  •     ci:test:stable

@liferay-continuous-integration
Copy link
Collaborator

Skipping previously passed test suites:
ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 47 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 1110b647c2539d8b3ba1fff788333bb9e010f4e9

Sender Branch:

Branch Name: LPS-125413-migrate-image-selector.2
Branch GIT ID: 5c0eaea602397ca54c3e71570e129e3a4f33e102

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

All required test suite(s) passed.
Forwarding pullrequest to brianchandotcom.

@liferay-continuous-integration
Copy link
Collaborator

Pull request has been successfully forwarded to brianchandotcom#98928

@liferay-continuous-integration
Copy link
Collaborator

@liferay-continuous-integration
Copy link
Collaborator

@carloslancha carloslancha deleted the LPS-125413-migrate-image-selector.2 branch February 24, 2023 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants