(#4389) NCIDS Flag Card Group

dev-rana-publicis commented 2 months ago

Closes #4389 Closes #4460 Closes #4455 ODE: https://ncigovcdode829.prod.acquia-sites.com/

Testing Links:

cgdp-management-server[bot] commented 2 months ago

ODE Deployment

Code has been deployed to ODE 829.

dlescarbeau commented 1 month ago

Front-end Feedback Addressed (9/24/24):

For breakpoints larger than Mobile Large, the padding on above the Card Title should be 16px per the comp, not 24px
For the Mobile Large and Mobile breakpoints, the padding on the top and bottom should be 20px, and 32px on the left. I'm assuming it should be 32px on the right too
Hover State zooms the image
From the comp, the cards should go full width for the Desktop, Tablet Lg, and Tablet breakpoints. It's currently staying in the two columns through Desktop
From the comp on Widescreen, the text should always be top aligned (including the padding). So cards with just the title should still have a top aligned title and when the height of the card gets very tall because of a lot of text, the card next to it with less text should have its title and text remain top aligned per the comp
The vertical space between cards should decrease from 32px to 24px when the image disappears on Mobile Lg and Mobile. I'm not seeing it explicitly on the comp, but can be seen in developer mode

Drupal/Other Fixes Addressed - DEV (9/24):

Addressed #4455
Set "NCIDS Card Internal" as the default option
Changed field title to "Flag Cards"
Addressed Analytics changes mentioned below
Changed view mode to ncids-flag-group-card

andyvanavery31 commented 1 month ago

Changes requested in Slack from 20SEP2024:

[x] Set the "Add NCIDS Card Internal" as the default option for the dropdown of adding cards to the group. (Name updated to what the paragraph will be labeled in the associated CR
[x] - Update padding above the Card Title to 16px on breakpoints greater than Mobile Large
[x] - Update the padding on top and bottom to be 20px and 32px on left and right for Mobile Large and Mobile breakpoints.
[x] - Update the hover state to have the image zoom like the feature card without the card increasing in size
[x] - Update the breakpoint behavior for the cards to go full width on Desktop, Tablet Large, and Tablet
[x] - When the cards are in the two-column layout, the text should be top aligned
[x] - When the cards are full-width, the text should be vertically center aligned
- [x] UPDATE 25SEP2024: When the cards are on Tablet Lg and Tablet, the padding on top and bottom of the text will be 16px each
- [x] UPDATE 25SEP2024: The padding between the Title and the Description will be moved from being placed on the title to being placed on the description
[x] - When the breakpoint is Mobile Large or Mobile, the image disappears, and the vertical space between the cards needs to be updated to 24px
[x] - Remove the shadow on the Flag Cards when the breakpoint is Mobile Large or Mobile (the image is not shown)
[x] - Confirm all cards in a group have a vertical spacing of 24px between them when at the Mobile Large or Mobile breakpoints
[x] - Change the field title from "Row Cards Unlimited" to "Flag Cards"
Note: The dropdown field to add Flag Cards currently shows "Add NCIDS Feature Card Internal/External/Multimedia" since this is a reused field. This will be updated to "Add NCIDS Card Internal/External/Multimedia" in a separate change request.
[x] UPDATE 25SEP2024: The title of the cards will be changed from an <h3> tag to a <span> tag to match functionality of the Feature Cards

welshja commented 1 month ago

@dev-rana-publicis, found a couple things in analytics testing. 2 issues and then 1 requirements update that I missed as headings got added as optional: Issues found

linkText is setting at “Not Defined” Per requirements this should be set to the title of the card. (The value that is currently passing as the “title” value is what we should set as linkText).
linkArea should be set to “Card” per requirements

Requirements Change:

Update "title" value to be set based on the optional heading field truncated at 50 characters. If this field is filled in, we should get this value (even if hidden). If it is not filled, we should pass “Not Defined”

welshja commented 1 month ago

@dev-rana-publicis, seeing 2 issues fixed, but the update to the requirements was not addressed.

Issue outstanding:

Update "title" value to be set based on the optional heading field truncated at 50 characters. If this field is filled in, we should get this value (even if hidden). If it is not filled, we should pass “Not Defined”

Issues resolved:

linkText is setting at “Not Defined” Per requirements this should be set to the title of the card. (The value that is currently passing as the “title” value is what we should set as linkText).
linkArea should be set to “Card” per requirements

dlescarbeau commented 1 month ago

9/25 Fixes:

changed <h3 class="cgdp-flag-card__title" id="{{ titleID }}">{{ title }}</h3> to be <span class="cgdp-flag-card__title" id="{{ titleID }}">{{ title }}</span> to match Feature Cards.
removed nci-card-group
removed nci-card and instead extends it where appropriate in the CSS
updated yaml content to remove "fifth" card verbiage
updated flag cards to become two columns at desktop rather than widescreen
applies padding-top to the description rather than padding-bottom to the title
updated padding values so vertical alignment is properly centered
slows image zoom transition on hover
Fixed test page to show the 4th card
Fixed analytics to get the title of the flag card group
4455 - Changed the description of NCIDS Card External/Internal/Multimedia. Please confirm that the new description looks good and will need to update the content model @andyvanavery31
Fixed cgdp-flag-card-group.dom.ts to not include nci-card styles.

bennettcc commented 1 month ago

Product reviewers + Fed team have passed this PR as of 9/27: adding the product check mark

NCIOCPL / cgov-digital-platform