Skip to content

Comments

feat(Modal)!: add multiview options for back and danger variant#3067

Merged
codecademydev merged 9 commits intomainfrom
ajr-modal-updates
Apr 21, 2025
Merged

feat(Modal)!: add multiview options for back and danger variant#3067
codecademydev merged 9 commits intomainfrom
ajr-modal-updates

Conversation

@aresnik11
Copy link
Contributor

@aresnik11 aresnik11 commented Apr 14, 2025

Overview

Adds bi-directional option for multiview modals, refactors the CTAs in multiview modals, and adds danger variant. Adds unit tests and additional stories.

PR Checklist

  • Related to designs:
  • Related to JIRA ticket: GM-1098
  • I have run this code to verify it works
  • This PR includes unit tests for the code change
  • This PR includes testing instructions tests for the code change
  • The alpha package of this PR is passing end-to-end tests in all relevant Codecademy repositories

Testing Instructions

  1. View the Modal story
  2. See all the new multiview examples!
  3. Optional: locally test out different primaryCta/secondaryCta combos and see TS do its job

PR Links and Envs

Repository PR Link PR Env
Monolith Monolith PR Monolith Env
Mono Portal Link Portal Env

@nx-cloud
Copy link

nx-cloud bot commented Apr 14, 2025

View your CI Pipeline Execution ↗ for commit 661360c.


☁️ Nx Cloud last updated this comment at 2025-04-17 19:59:12 UTC

@aresnik11 aresnik11 changed the title Ajr modal updates feat(Modal)!: add multiview options for back and danger variant Apr 15, 2025
@aresnik11 aresnik11 marked this pull request as ready for review April 15, 2025 17:26
@aresnik11 aresnik11 requested a review from a team as a code owner April 15, 2025 17:26
Copy link
Contributor

@LinKCoding LinKCoding left a comment

Choose a reason for hiding this comment

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

Left some thoughts, but nothing blocking :)

Love the new examples and tests!!

onRequestClose: mockRequestClose,
});

describe('Modal', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

😂 lol nice catch

});

describe('Multi view', () => {
it('renders modal title and view children when modal title is provided', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

great, test — it made me think that it'd be helpful to callout this info in the story as well.

In the #Multi View section, maybe something like:
Each view can also take a title that renders as a heading. However, if the Modal already has a title in the parent element then that value will be the title rendered instead of the view's title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure! yeah this was confusing when i was writing the test

Copy link
Contributor

@dreamwasp dreamwasp left a comment

Choose a reason for hiding this comment

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

looks great, love the new tests + views

@codecademydev
Copy link
Collaborator

📬Published Alpha Packages:

@codecademy/gamut@61.1.3-alpha.661360.0
@codecademy/gamut-kit@0.6.497-alpha.661360.0
@codecademy/styleguide@72.1.1-alpha.661360.0

@codecademydev
Copy link
Collaborator

🚀 Styleguide deploy preview ready!

https://68015db36843857bad551e72--gamut-preview.netlify.app

Deploy Logs

@aresnik11 aresnik11 added the Ship It :shipit: Automerge this PR when possible label Apr 21, 2025
@codecademydev codecademydev merged commit 567dcac into main Apr 21, 2025
20 of 21 checks passed
@codecademydev codecademydev removed the Ship It :shipit: Automerge this PR when possible label Apr 21, 2025
@codecademydev codecademydev deleted the ajr-modal-updates branch April 21, 2025 14:32
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.

4 participants