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

SITES-22935 NGDM remote image issue #2792

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

mohiaror
Copy link
Contributor

Make changes work for multiple fileupload instances present in a custom multifield component.

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Minor: New Feature?
Major: Breaking Change?
Tests Added + Pass? Yes
Documentation Provided Yes (code comments and or markdown)
Any Dependency Changes?
License Apache License, Version 2.0

Make changes work for multiple fileupload instances present in a custom multifield component.
@mohiaror mohiaror requested review from YahorC and LSantha June 20, 2024 15:15
@mohiaror
Copy link
Contributor Author

@YahorC I could not understand the change done in this commit - bfc2bd6

I am not sure if that change is related to remoteReference (which are used for polaris' connected assets usecase). I tried to incorporate that fix in my PR. Can you please check if it is Ok?

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.21%. Comparing base (b5e6a56) to head (f97f697).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2792   +/-   ##
=========================================
  Coverage     87.21%   87.21%           
  Complexity     2687     2687           
=========================================
  Files           235      235           
  Lines          7170     7170           
  Branches       1096     1096           
=========================================
  Hits           6253     6253           
  Misses          363      363           
  Partials        554      554           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Incorporate review comments.
@LSantha
Copy link
Contributor

LSantha commented Jun 25, 2024

@mohiaror , could you please add UI test for this fix?

@YahorC
Copy link
Contributor

YahorC commented Jun 25, 2024

@mohiaror Changes that I applied was regarding page properties image section
So for Image component everything was working fine, but page properties sharing the same logic, but node structure, to which we are referring while trying to fetch image path are different for page properties Image section and just image component
Generally fix is looking fine for me, but could you test please, if it works fine for page properties image tab, to make sure it is working fine https://jira.corp.adobe.com/browse/SITES-21939

Copy link

sonarqubecloud bot commented Jul 1, 2024

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.

4 participants