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

feat(Modal): add large size option to Modal #2883

Merged
merged 4 commits into from
Jun 7, 2024
Merged

Conversation

sooinc
Copy link
Contributor

@sooinc sooinc commented Jun 6, 2024

Overview

  • adds large size option to Modal component

PR Checklist

  • Related to designs: https://www.figma.com/design/h5Opgw8Inv1KSSsek0aawR/%F0%9F%8C%A0-Skill-XP?node-id=259-7826&m=dev (the width may not be adjusted in the figma but spoke to jerimie, and we landed on 680px for width to follow the same difference between small and medium size widths)
  • Related to JIRA ticket: [ABC-123]
  • 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. Check modal page: http://localhost:6006/?path=/docs/molecules-modals-modal--large
Screenshot 2024-06-06 at 5 05 42 PM

PR Links and Envs

Repository PR Link PR Env

| Portal | Portal Link | Portal Env |

@sooinc sooinc changed the title add large size option to Modal feat(Modal): add large size option to Modal Jun 6, 2024
@sooinc sooinc marked this pull request as ready for review June 6, 2024 21:10
@sooinc sooinc requested a review from a team as a code owner June 6, 2024 21:10
@sooinc sooinc requested review from jakemhiller, LinKCoding and dreamwasp and removed request for jakemhiller June 6, 2024 21:10
@LinKCoding
Copy link
Contributor

Hi @sooinc -- I checked with Stacey and she noticed that the link designs don't have any Modal that are 680px wide. Will the end result need this width? And curious if these dimensions couldn't be achieved using the fluid variant?

@sooinc
Copy link
Contributor Author

sooinc commented Jun 7, 2024

Hi @sooinc -- I checked with Stacey and she noticed that the link designs don't have any Modal that are 680px wide. Will the end result need this width? And curious if these dimensions couldn't be achieved using the fluid variant?

Hey @LinKCoding , the design hasnt been updated yet but i noted in my overview that I spoke to jerimie, and we landed on 680px for width to follow the same difference between small and medium size widths. Sorry I shouldve called out that I tried to achieve this with fluid variant first but i wasnt able to as I know fluid takes the width of the child but giving the child minWidth or width, the contents (tabs / expandable) does not minimize accordingly and maxWidth does not take the full size of the 680px that we want.

I could def not be thinking of something though i have looked at diff combination of width for a bit lol so lmk if there's a way to make it work with fluid.
Screenshot 2024-06-07 at 1 00 02 PM
Screenshot 2024-06-07 at 12 57 00 PM

@LinKCoding
Copy link
Contributor

LinKCoding commented Jun 7, 2024

Hi @sooinc -- I checked with Stacey and she noticed that the link designs don't have any Modal that are 680px wide. Will the end result need this width? And curious if these dimensions couldn't be achieved using the fluid variant?

Hey @LinKCoding , the design hasnt been updated yet but i noted in my overview that I spoke to jerimie, and we landed on 680px for width to follow the same difference between small and medium size widths. Sorry I shouldve called out that I tried to achieve this with fluid variant first but i wasnt able to as I know fluid takes the width of the child but giving the child minWidth or width, the contents (tabs / expandable) does not minimize accordingly and maxWidth does not take the full size of the 680px that we want.

I could def not be thinking of something though i have looked at diff combination of width for a bit lol so lmk if there's a way to make it work with fluid.

Gotcha, thanks :) sorries I blanked on that and I should've also checked your portal PR. It seems like Jeremie didn't confirm with Stacey, but since you and Jeremie have aligned and the design's intentional then it's all good

Stacey will cut a ticket to update the Modal (and Dialog) component on Figma and code looks good :)
You'll need to yarn format before merging but other than that 👍

@sooinc
Copy link
Contributor Author

sooinc commented Jun 7, 2024

Thanks for the review! And that's my bad, I didnt realize i needed to let Stacey know to cut a separate figma ticket. Thanks for letting her know 🙏

@LinKCoding
Copy link
Contributor

Thanks for the review! And that's my bad, I didnt realize i needed to let Stacey know to cut a separate figma ticket. Thanks for letting her know 🙏

Ah no, not you lol Jeremie (I think Design has their own flow, cause ultimately, Stacey will need to update the Gamut Figma doc) so usually with Gamut updates, Designers should let Stacey know about changes.

I was just double-checking everything cause it's all new to me and happened to catch this miscommunication of sorts.

@codecademydev
Copy link
Collaborator

📬Published Alpha Packages:

@codecademy/gamut@55.21.3-alpha.524193.0
@codecademy/gamut-kit@0.6.412-alpha.524193.0
@codecademy/styleguide@66.22.1-alpha.524193.0

@codecademydev
Copy link
Collaborator

🚀 Styleguide deploy preview ready!

https://66636459879b552484bed7b6--gamut-preview.netlify.app

Deploy Logs

@sooinc sooinc added the Ship It :shipit: Automerge this PR when possible label Jun 7, 2024
@codecademydev codecademydev merged commit a5f97f9 into main Jun 7, 2024
17 of 18 checks passed
@codecademydev codecademydev deleted the sc-modal-add-lg-size branch June 7, 2024 20:05
@codecademydev codecademydev removed the Ship It :shipit: Automerge this PR when possible label Jun 7, 2024
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.

3 participants