Skip to content

Conversation

@bozojovicic
Copy link
Contributor

@bozojovicic bozojovicic commented Apr 9, 2025

Plans card for students tab. Will be used outside card collection. Requested width 568px.
Authored in MAS Studio with new variant "Plans students".

Test Milo page https://mwpw164493students--milo--bozojovicic.aem.page/drafts/bozo/students?martech=off
MAS PR mas/pull/236
MAS studio test page mwpw-164493--mas--adobecom.aem.live/studio.html?milolibs=mwpw164493students--milo--bozojovicic#path=sandbox&page=content&query=78856cda-ba13-424d-ac46-be79ac6545cb

Resolves: MWPW-164493

Test URLs:

@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented Apr 9, 2025

@bozojovicic bozojovicic marked this pull request as ready for review April 9, 2025 12:11
width: var(--consonant-merch-card-plans-width);
}
merch-card[variant="plans"][size="students"] {
Copy link
Contributor

Choose a reason for hiding this comment

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

please do not use size field for this. You need to add a new attribute like customer-segment

Copy link
Contributor Author

@bozojovicic bozojovicic Apr 10, 2025

Choose a reason for hiding this comment

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

Customer segment? You probably mean Market segment (COM / EDU)?
Customer segment is INDIVIDUAL / TEAM, that would be probably needed for business cards.

So @yesil we will need another field in odin for market segment and new dropdown in card editor with values for COM and EDU, right?

Actually, instead of COM/EDU we better have options like : Individuals, Business, Students, Universities.

You ruined mine and @3ch023's idea to make this extremely simple. :)

Copy link
Contributor

@yesil yesil Apr 10, 2025

Choose a reason for hiding this comment

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

@bozojovicic sorry, width="students" is a no go for me.
we can also create plans-edu variant, and re-use the same plans variant code without having to add a new field.
cc: @3ch023

Copy link
Contributor

Choose a reason for hiding this comment

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

we can, but isn't it too much code to just have a slightly different card width?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • we don't add a new field, just a new 'width' variation

Copy link
Contributor Author

@bozojovicic bozojovicic Apr 11, 2025

Choose a reason for hiding this comment

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

I created new variant "Plans students" for this card. It inherits Plans variant completely and adds new attribute segment="students" to the card so we can add styles for this card (which is for now only the card width) and reuse all styles of the plans card.

Screenshot 2025-04-11 at 14 47 16

allowedBorderColors: ['spectrum-yellow-300-plans', 'spectrum-gray-300-plans'],
borderColor: { attribute: 'border-color' },
size: ['wide', 'super-wide'],
size: ['wide', 'super-wide', 'students'],
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there is a wide Student card? We should not misuse fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

we add 'studebts-wide' option then

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 had a call with Ilyas and he explained his ideas so this comment is no longer relevant. It is not much different from what I already did. The biggest difference is - I rebased this PR to his branch MWPW-170799.

@3ch023
Copy link
Contributor

3ch023 commented Apr 14, 2025

@bozojovicic pls add a marquee to your test page with a card and use it in before/after instead of a start page


await test.step('step-2: Verify Plans Students Merch Card CSS', async () => {
await expect(acomPage.getCard(data.id)).toBeVisible();
await expect(acomPage.getCard(data.id)).toHaveAttribute('segment', 'students');
Copy link
Contributor

Choose a reason for hiding this comment

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

cool!

@bozojovicic bozojovicic changed the base branch from stage to MWPW-170799 April 14, 2025 12:32
padding: 2px 10px 3px;
}
:host([variant='plans'][segment='students']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need segment='students' ? should we simply use host([variant='plans-edu'] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yesil With host([variant='plans-edu'] none of those styles for merch-card[variant="plans"] will not be applied (and we need them for students card as well) so I either need to do add segment='students' attribute to the card or I would need to add merch-card[variant="plans-edu"] to every single CSS selector merch-card[variant="plans"] which is not a solution really.

Copy link
Contributor

Choose a reason for hiding this comment

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

what if we change them from :host([variant='plans']) to : :host([variant^='plans'])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Done with merch-card[variant^="plans"] and merch-card[variant="plans-edu"]
No more segment attribute.

ANALYTICS_SECTION_ATTR,
];
attributesToRemove.forEach(attr => merchCard.removeAttribute(attr));
if (variant === 'plans') merchCard.removeAttribute('segment');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (variant === 'plans') merchCard.removeAttribute('segment');
merchCard.removeAttribute('segment');

no need to check for a variant, just always delete it

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 can't. If I do that without this check I will lose styles for students card. Let me explain why I need this :
we open the card editor in mas for students card
when I change the variant from "plans students" to "plans" I need to remove the "segment" attribute
otherwise styles for students card will be applied although we changed it to plans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted. :)

@Roycethan
Copy link
Contributor

@bozojovicic
How to format "Exclusive of VAT. Annual, paid monthly." so that they are placed below the Price in same line with Body XS, 400, 14px formatting as in figma ?
I tried adding in Prices field but the formatting is not matched and not moved to next line
image

Expected:
image

@afmicka
Copy link
Contributor

afmicka commented Apr 17, 2025

@bozojovicic How to format "Exclusive of VAT. Annual, paid monthly." so that they are placed below the Price in same line with Body XS, 400, 14px formatting as in figma ? I tried adding in Prices field but the formatting is not matched and not moved to next line

@Roycethan @bozojovicic that should be enabled with Ilyas's PR (but not ready yet): #3971
Not yet possible unless you put that text manually in the description which then separates it from the price but...

@milo-pr-merge milo-pr-merge bot deleted the branch adobecom:MWPW-170799 April 17, 2025 09:18
@milo-pr-merge milo-pr-merge bot closed this Apr 17, 2025
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.

5 participants