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

MWPW-144813: Add 'xl-size' class for dialog-modal #2048

Merged

Conversation

mirafedas
Copy link
Contributor

@mirafedas mirafedas commented Mar 20, 2024

This change adds a new xl-size class for the modal window, which allows the modal window to fill all available screen height on mobile and tablet, and to be wider on desktop.
We set this class to the modal window that renders an iframe with the following pages:
https://www.adobe.com/creativecloud/whats-included/mini-plans/cci-all-apps-whats-included.html
https://www.adobe.com/creativecloud/whats-included/mini-plans/edu-all-apps-whats-included.html
https://www.adobe.com/creativecloud/whats-included/mini-plans/cct-all-apps-whats-included.html
https://www.adobe.com/creativecloud/whats-included/plans/cci-all-apps-whats-included.html
https://www.adobe.com/creativecloud/whats-included/plans/edu-all-apps-whats-included.html
https://www.adobe.com/creativecloud/whats-included/plans/cct-all-apps-whats-included.html
We may use it later for other use cases as well.

Resolves: MWPW-144813

Test URLs:

Note:
Make sure the ModHeaders plugin has the frame-ancestors set to * in the CSP response modifiers, without this on the 'After' page the modal content will not render because of the CORS.

Copy link
Contributor

aem-code-sync bot commented Mar 20, 2024

Page Scores Audits Google
/drafts/mirafedas/all-modals PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@mirafedas mirafedas added run-nala Run Nala Test Automation against PR @modal Run Modal Tests Nala needs-verification PR requires E2E testing by a reviewer labels Mar 20, 2024
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.41%. Comparing base (94d3586) to head (7d53b31).

Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #2048      +/-   ##
==========================================
+ Coverage   96.38%   96.41%   +0.03%     
==========================================
  Files         166      166              
  Lines       43448    43448              
==========================================
+ Hits        41878    41892      +14     
+ Misses       1570     1556      -14     

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

.dialog-modal.xl .milo-iframe {
height: 100%;
padding-bottom: 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what are implicit values for those rules otherwise?
i'm noticing same setup for > 1200px and .dialog-modal.commerce-frame .milo-iframe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@npeltier
Without these rules:

  • for the .dialog-modal the height value is 'fit-content'.
  • for .fragment and .section we don't set values, so the default value ('auto') applies.
  • for .milo-iframe height is set to 0, and padding-bottom to 56.25%.

The class .commerce-frame should be used only on commerce related modal windows.
'What's Included' content is not related to commerce, so it makes sense to use a more general class name.

libs/blocks/modal/modal.js Outdated Show resolved Hide resolved
@mirafedas mirafedas force-pushed the mwpw-144813-whatsincluded-styles branch 2 times, most recently from 0b7febf to 7122434 Compare March 20, 2024 15:38
@afmicka afmicka added verified PR has been E2E tested by a reviewer and removed needs-verification PR requires E2E testing by a reviewer labels Mar 25, 2024
@mirafedas mirafedas added run-nala Run Nala Test Automation against PR and removed @modal Run Modal Tests Nala run-nala Run Nala Test Automation against PR labels Mar 25, 2024
@Roycethan Roycethan self-requested a review March 27, 2024 00:32
Copy link

@Roycethan Roycethan left a comment

Choose a reason for hiding this comment

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

@mirafedas
Copy link
Contributor Author

@Roycethan the modal you are checking has classes dialog-modal commerce-frame. We agreed that all non-commerce-related modals should not have the commerce-frame class, I guess the authors just didn't remove that class yet. To test the current change the modal should have classes dialog-modal xl, then it will be wider on desktop.
cc @afmicka

Copy link
Contributor

@overmyheadandbody overmyheadandbody left a comment

Choose a reason for hiding this comment

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

Just a few notes:

  • the modal content doesn't load on the After link, it might be worth checking out;
  • if I'm remembering correctly, we don't usually have just .<size> classes, they-re attached to some other meaning, say .<size>-spacing. Do you have another reference to such classes, are we using this pattern anywhere else? We want to keep things as consistent as possible for authors, so this might deserve some attention. An author could interpret a simple xl as a font choice for example, the purpose should be clear

@mirafedas mirafedas force-pushed the mwpw-144813-whatsincluded-styles branch from 400a9e7 to 647b864 Compare March 27, 2024 12:54
@mirafedas
Copy link
Contributor Author

mirafedas commented Mar 27, 2024

@overmyheadandbody
good point, I renamed xl > xl-size.
Do you have ModHeaders plugin enabled with the frame-ancestors set to * in the CSP response modifier? Without that due to CORS the modal content does not load. I just added this info to the PR description, thanks for the reminder :)

@mirafedas mirafedas force-pushed the mwpw-144813-whatsincluded-styles branch from 647b864 to 7d53b31 Compare March 27, 2024 13:13
@overmyheadandbody
Copy link
Contributor

Hey @mirafedas! Thanks for the changes! I just saw another PR, suggesting .size-s as a class name, could you consolidate and use one or the other?

@mirafedas
Copy link
Contributor Author

@overmyheadandbody yes, doing that right now :)

@mirafedas mirafedas changed the title MWPW-144813: Add 'xl' class for dialog-modal MWPW-144813: Add 'xl-size' class for dialog-modal Mar 27, 2024
@narcis-radu narcis-radu merged commit b2c9720 into adobecom:stage Apr 3, 2024
11 checks passed
rgclayton added a commit that referenced this pull request Apr 4, 2024
rgclayton added a commit that referenced this pull request Apr 4, 2024
Revert "MWPW-144813: Add 'xl-size' class for dialog-modal (#2048)"

This reverts commit b2c9720.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for Stage run-nala Run Nala Test Automation against PR verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants