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.

Sign up for GitHub

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

Jump to bottom

drag and drop feature in file input #935

Closed
wants to merge 1 commit into from
Closed

drag and drop feature in file input #935

wants to merge 1 commit into from

Conversation

rajatofficial
Copy link
Contributor

No description provided.

@rajatofficial rajatofficial changed the title drag and dropp intial commit drag and dropp initial Oct 19, 2023
@rajatofficial rajatofficial changed the title drag and dropp initial drag and drop feature in file input Oct 19, 2023
@rajatofficial rajatofficial force-pushed the dragDropV2 branch 4 times, most recently from c296e07 to 6f2ba76 Compare October 20, 2023 07:22
@barshat7
Copy link
Contributor

Please fix the test failures and check comments

@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 90 100 100 73

@adobe-bot
Copy link

Lighthouse scores (desktop)

Performance Accessibility Best-Practices SEO
Scores 100 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
label-title-only serious
landmark-one-main moderate
region moderate

@codecov
Copy link

codecov bot commented Oct 20, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (39d1ca1) 80.29% compared to head (627ec86) 80.34%.

❗ Current head 627ec86 differs from pull request most recent head 62c6b4e. Consider uploading reports for the commit 62c6b4e to get more accurate results

Additional details and impacted files 
@@             Coverage Diff              @@
##                dev     #935      +/-   ##
============================================
+ Coverage     80.29%   80.34%   +0.04%     
- Complexity      729      732       +3     
============================================
  Files            89       90       +1     
  Lines          2045     2050       +5     
  Branches        271      271              
============================================
+ Hits           1642     1647       +5     
  Misses          251      251              
  Partials        152      152

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

@rajatofficial rajatofficial force-pushed the dragDropV2 branch 3 times, most recently from 0c4d213 to f692b20 Compare October 27, 2023 05:34
@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 90 100 100 73

@adobe-bot
Copy link

Lighthouse scores (desktop)

Performance Accessibility Best-Practices SEO
Scores 100 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
label-title-only serious
landmark-one-main moderate
region moderate

@adobe-bot
Copy link

Lighthouse scores (desktop)

Performance Accessibility Best-Practices SEO
Scores 100 100 100 73

@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 89 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
label-title-only serious
landmark-one-main moderate
region moderate

rismehta
rismehta requested changes Nov 8, 2023
View reviewed changes
Copy link
Collaborator

@rismehta rismehta left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the following,

  1. I see a lot of copy/paste of code which should have being re-used.
  2. Please fix the property names.

i will do a second review once this is completed

...rc/main/java/com/adobe/cq/forms/core/components/internal/models/v2/form/FileInputImplV2.java
ComponentExporter.class },
resourceType = { FormConstants.RT_FD_FORM_FILE_INPUT_V2 })
@Exporter(name = ExporterConstants.SLING_MODEL_EXPORTER_NAME, extensions = ExporterConstants.SLING_MODEL_EXTENSION)
public class FileInputImplV2 extends FileInputImpl {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implements FileInput

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need fileinputimpl with one extra property that why extended the FileInputImpl. Should I write whole FileInputImplV2 similar to FileInputImpl by implementing FileInput and then adding my properties above that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to extend the base implementation, but adding an explicit implements makes the code more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CWIIW but as FileInputImplV2 is extending the FileInputImpl which is already implementing the FileInput hence its not requried to implement in V2 as well.

...rc/main/java/com/adobe/cq/forms/core/components/internal/models/v2/form/FileInputImplV2.java Outdated Show resolved Hide resolved
bundles/af-core/src/test/resources/schema/0.12.1/adaptive-form.schema.json Outdated Show resolved Hide resolved
...n/content/jcr_root/apps/forms-components-examples/components/form/fileinput/_cq_template.xml Outdated Show resolved Hide resolved
...content/jcr_root/apps/core/fd/components/form/fileinput/v2/fileinput/_cq_dialog/.content.xml Outdated Show resolved Hide resolved
...content/jcr_root/apps/core/fd/components/form/fileinput/v2/fileinput/_cq_dialog/.content.xml Show resolved Hide resolved
...s/src/main/content/jcr_root/apps/core/fd/components/form/fileinput/v2/fileinput/.content.xml Show resolved Hide resolved
..._root/apps/core/fd/components/form/fileinput/v2/fileinput/clientlibs/editor/js/editDialog.js Show resolved Hide resolved
@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 87 100 100 73

@adobe-bot
Copy link

Lighthouse scores (desktop)

Performance Accessibility Best-Practices SEO
Scores 100 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
label-title-only serious
landmark-one-main moderate
region moderate

@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 90 100 100 73

@adobe-bot
Copy link

Lighthouse scores (desktop)

Performance Accessibility Best-Practices SEO
Scores 100 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
label-title-only serious
landmark-one-main moderate
region moderate

rismehta
rismehta requested changes Dec 4, 2023
View reviewed changes
Copy link
Collaborator

@rismehta rismehta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design wise, the PR looks incorrect to me

@rajatofficial rajatofficial force-pushed the dragDropV2 branch 2 times, most recently from 77bd9f8 to ef6ed58 Compare December 14, 2023 06:46
@rajatofficial rajatofficial force-pushed the dragDropV2 branch 5 times, most recently from 64f6699 to 32bc644 Compare December 18, 2023 06:20
@rajatofficial rajatofficial force-pushed the dragDropV2 branch 2 times, most recently from 50859d5 to 32bc644 Compare December 18, 2023 07:20
rismehta
rismehta requested changes Dec 18, 2023
View reviewed changes
Copy link
Collaborator

@rismehta rismehta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barshat7 barshat7 barshat7 left review comments

@rismehta rismehta rismehta requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rajatofficial @barshat7 @adobe-bot @rismehta @rajatkhurana-adobe