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

Terms and Conditions Component #889

Merged
merged 5 commits into from
Oct 10, 2023

Conversation

barshat7
Copy link
Contributor

Terms and Conditions Component

Terms and Conditions component

Terms and conditions: code formatting and readme

Terms and conditions tooltip

Terms and conditions review comments

Terms and conditions review comments

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

@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 92 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

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

(function($) {
"use strict";

var Utils = window.CQ.FormsCoreComponents.Utils.v1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the latest ES6 syntax like let, const etc

jcr:primaryType="cq:Component"
jcr:title="Adaptive Form Links (v1)"
sling:resourceSuperType="core/fd/components/form/checkboxgroup/v1/checkboxgroup"
componentGroup=".core-adaptiveform"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The component group for this should be hidden, I don't see any usage of this other than TNC

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also should we name this as toggleable link ?

~ See the License for the specific language governing permissions and
~ limitations under the License.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/-->
<sly data-sly-use.renderer="${'checkboxgroup.js'}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need to copy paste the entire sightly, it should auto inherit from parent ? This will lead to maintenance hazard

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

@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 91 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 Sep 26, 2023

Codecov Report

Merging #889 (b1e26ec) into dev (08189ee) will increase coverage by 0.08%.
The diff coverage is 92.85%.

@@             Coverage Diff              @@
##                dev     #889      +/-   ##
============================================
+ Coverage     80.16%   80.25%   +0.08%     
- Complexity      718      728      +10     
============================================
  Files            88       89       +1     
  Lines          2032     2046      +14     
  Branches        269      271       +2     
============================================
+ Hits           1629     1642      +13     
  Misses          254      254              
- Partials        149      150       +1     
Files Coverage Δ
...s/core/components/internal/form/FormConstants.java 100.00% <ø> (ø)
...nternal/models/v1/form/TermsAndConditionsImpl.java 92.85% <92.85%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 93 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

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


String FD_TERMS_AND_CONDITIONS = "fd:tnc";

boolean isShowApprovalOption();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add java doc for all API's and also update package-info

@github-actions github-actions bot force-pushed the terms-and-conditions-feature_singlecommit branch from 97375b1 to 3ca93e5 Compare October 2, 2023 08:36
@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 88 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

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

@barshat7 barshat7 force-pushed the terms-and-conditions-feature_singlecommit branch from c48c506 to 5673178 Compare October 3, 2023 06:08
@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 90 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 89 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 88 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

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

@barshat7 barshat7 force-pushed the terms-and-conditions-feature_singlecommit branch from 5673178 to fa1bdc8 Compare October 9, 2023 06:52
@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 91 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

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

fieldLabel="Link"
fieldDescription="Enter the link."
required="{Boolean}true"/>
<enumNames
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you implement the rich text which was recently added by @im-shiv, its missing here ? Please also add test cases for the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already done

${checkboxgroup.enumNamesAsTextContent[itemList.index].value @ context = checkboxgroup.enumNamesAsTextContent[itemList.index].richText ? 'html' : 'text'}

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

* Defines the form {@code TermsAndConditions} Sling Model used for the
* {@code /apps/core/fd/components/form/termsandconditions/v1/termsandconditions} component.
*
* @since com.adobe.cq.forms.core.components.models.form 4.5.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update the package-info.java with 4.7.0 and update the same here, since new interface is added

@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 91 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 86 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

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

~ limitations under the License.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/-->
<template data-sly-template.widget="${ @ checkboxgroup }">
<div class="cmp-adaptiveform-checkboxgroup__widget ${checkboxgroup.orientation}" id="${widgetId}">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this copy pasted ? Ideally, markup needs to be re-used from checkbox group right ?

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.

-1 for now

@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 91 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

barshatr added 5 commits October 9, 2023 14:17
Terms and Conditions Component

Terms and Conditions Component

Terms and Conditions component

Terms and conditions: code formatting and readme

Terms and conditions tooltip

Terms and conditions review comments

Terms and conditions review comments

Terms and Conditions Test Case Fix

Terms and Conditions added new tests

Terms and Conditions review comments

Terms and Conditions component
@github-actions github-actions bot force-pushed the terms-and-conditions-feature_singlecommit branch from 6685b0c to b1e26ec Compare October 9, 2023 14:19
@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 91 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

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

@rismehta rismehta merged commit f46f89a into dev Oct 10, 2023
4 checks passed
@rismehta rismehta deleted the terms-and-conditions-feature_singlecommit branch October 10, 2023 06:24
barshat7 added a commit that referenced this pull request Oct 13, 2023
* Terms and Conditions

Terms and Conditions Component

Terms and Conditions Component

Terms and Conditions component

Terms and conditions: code formatting and readme

Terms and conditions tooltip

Terms and conditions review comments

Terms and conditions review comments

Terms and Conditions Test Case Fix

Terms and Conditions added new tests

Terms and Conditions review comments

Terms and Conditions component

* Terms and Conditions

* Terms and Conditions

* Terms and conditions: Rich Text Support

* Terms and conditions: bump model version

---------

Co-authored-by: barshatr <[email protected]>
barshat7 added a commit that referenced this pull request Oct 13, 2023
* Terms and Conditions

Terms and Conditions Component

Terms and Conditions Component

Terms and Conditions component

Terms and conditions: code formatting and readme

Terms and conditions tooltip

Terms and conditions review comments

Terms and conditions review comments

Terms and Conditions Test Case Fix

Terms and Conditions added new tests

Terms and Conditions review comments

Terms and Conditions component

* Terms and Conditions

* Terms and Conditions

* Terms and conditions: Rich Text Support

* Terms and conditions: bump model version

---------

Co-authored-by: barshatr <[email protected]>
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.

3 participants