Skip to content

FORMS-26630: Add show as popup property in guidecontainer model json - #1927

Closed
akshayvas wants to merge 3 commits into
adobe:devfrom
akshayvas:fork_dev
Closed

FORMS-26630: Add show as popup property in guidecontainer model json#1927
akshayvas wants to merge 3 commits into
adobe:devfrom
akshayvas:fork_dev

Conversation

@akshayvas

Copy link
Copy Markdown
Contributor

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.

@akshayvas akshayvas closed this Jul 23, 2026
@akshayvas akshayvas reopened this Jul 23, 2026

@sakshi-arora1 sakshi-arora1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@akshayvas Where is the implementation honouring this configuration?

@sakshi-arora1 sakshi-arora1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overview: adds fd:showAsPopup to the exported properties of TermsAndConditionsImpl so consumers of the JSON model can tell whether the T&C panel should render as a popup. Note the PR title mentions "guidecontainer" but the diff only touches TermsAndConditionsImpl — worth fixing the title/description for traceability.

Main concern is inline below: the new property is gated behind an unrelated existing condition (fd:tnc presence), which will silently drop fd:showAsPopup from the export for any content that doesn't have that marker property set.

Test coverage: both updated fixtures (exporter-termsandconditions.json, exporter-termsandconditionsNoWrapData.json) and the underlying test-content.json already have fd:tnc: true hardcoded, so there's no test exercising the branch where fd:tnc is absent — i.e. no test proves/documents whether fd:showAsPopup is expected to disappear in that case.

Map<String, Object> properties = super.getProperties();
if (resource.getValueMap().containsKey(CUSTOM_TNC_PROPERTY)) {
properties.put(CUSTOM_TNC_PROPERTY, true);
properties.put(SHOW_AS_POPUP_PROPERTY, isShowAsPopup());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This ties fd:showAsPopup export to the presence of the fd:tnc resource property, which represents an unrelated concept (custom T&C content marker). isShowAsPopup() is independent of whether fd:tnc is set.

In practice fd:tnc is stamped by the component's _cq_template on newly authored nodes, but any existing/legacy T&C content authored before that (or content created without going through the template) won't have fd:tnc on the resource. For that content, fd:showAsPopup will simply be missing from the exported JSON even when "Show as popup" is checked in the dialog, so the runtime has no way to know it should render as a popup.

Suggest moving this line (and probably the whole showAsPopup property) outside the if (resource.getValueMap().containsKey(CUSTOM_TNC_PROPERTY)) block so it's always emitted, e.g.:

properties.put(SHOW_AS_POPUP_PROPERTY, isShowAsPopup());
if (resource.getValueMap().containsKey(CUSTOM_TNC_PROPERTY)) {
    properties.put(CUSTOM_TNC_PROPERTY, true);
}

@akshayvas

Copy link
Copy Markdown
Contributor Author

@akshayvas Where is the implementation honouring this configuration?

@sakshi-arora1
Implementation In headless - adobe/aem-forms-headless-components#179

@akshayvas

Copy link
Copy Markdown
Contributor Author

@sakshi-arora1 I have addressed review comments in below PR, closing this, as current PR is failing to sync branch from fork. It will have only exporter json baseline test failure due to addition of new property.

#1929

@akshayvas akshayvas closed this Jul 29, 2026
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.

2 participants