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.

Sign up for GitHub

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

Jump to bottom

feat(Dialog): a new optional prop for the close button #2916

Merged
merged 6 commits into from
Aug 14, 2024
Merged

feat(Dialog): a new optional prop for the close button #2916

merged 6 commits into from
Aug 14, 2024

Conversation

ITZSHOAIB
Copy link
Contributor

@ITZSHOAIB ITZSHOAIB commented Jul 31, 2024 •
edited
Loading

Overview

Added a new optional prop for the close button related to its tip.

PR Checklist

  • Related to designs:
  • Related to JIRA ticket: ENT-233
  • 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

PR Links and Envs

Repository PR Link PR Env
LE Enterprise LE Enterprise Link LE Enterprise Env

@ITZSHOAIB ITZSHOAIB requested a review from a team as a code owner July 31, 2024 16:16
@ITZSHOAIB ITZSHOAIB requested review from BandanaKM and aresnik11 July 31, 2024 16:16
@CLAassistant
Copy link

CLAassistant commented Jul 31, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ITZSHOAIB ITZSHOAIB requested a review from dreamwasp July 31, 2024 16:16
dreamwasp
dreamwasp reviewed Jul 31, 2024
View reviewed changes
packages/gamut/src/Modals/Dialog.tsx Outdated
@@ -69,12 +73,12 @@ export const Dialog: React.FC<DialogProps> = ({
</Text>
<Box gridArea="close">
<IconButton
aria-label="Close Dialog"
aria-label={closeButtonLabel}
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you can delete this line, i don't think it needs an aria-label, the tip text should be enough (it auto-adds aria-label based on tip text)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in the latest commit. Thanks!

@ITZSHOAIB ITZSHOAIB requested a review from dreamwasp July 31, 2024 16:33
@ITZSHOAIB ITZSHOAIB changed the title feat(Dialog): two new optional props for the close button feat(Dialog): a new optional prop for the close button Jul 31, 2024
dreamwasp
dreamwasp reviewed Aug 1, 2024
View reviewed changes
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.

this looks great! could you open a PR with the gamut-kit alpha package? there are instructions here on how to do that. we just like to make sure nothing is breaking + all tests are passing.

will ✅ once that's done!

@ITZSHOAIB
Copy link
Contributor Author

this looks great! could you open a PR with the gamut-kit alpha package? there are instructions here on how to do that. we just like to make sure nothing is breaking + all tests are passing.

will ✅ once that's done!

Hi @dreamwasp, here's a PR that uses this alpha version: https://github.com/codecademy-engineering/mono/pull/7110

dreamwasp
dreamwasp approved these changes Aug 1, 2024
View reviewed changes
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!

@dreamwasp dreamwasp added the Ship It :shipit: Automerge this PR when possible label Aug 14, 2024
@codecademydev
Copy link
Collaborator

📬Published Alpha Packages:

@codecademy/[email protected]
@codecademy/[email protected]
@codecademy/[email protected]

@codecademydev codecademydev merged commit 54eaddc into main Aug 14, 2024
16 of 18 checks passed
@codecademydev codecademydev removed the Ship It :shipit: Automerge this PR when possible label Aug 14, 2024
@codecademydev codecademydev deleted the ss-dialog-props branch August 14, 2024 13:40
@codecademydev
Copy link
Collaborator

🚀 Styleguide deploy preview ready!

https://66bcb41e665218ba5fb2f0c3--gamut-preview.netlify.app

Deploy Logs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamwasp dreamwasp dreamwasp approved these changes

@BandanaKM BandanaKM Awaiting requested review from BandanaKM BandanaKM is a code owner automatically assigned from Codecademy/gamut

@aresnik11 aresnik11 Awaiting requested review from aresnik11 aresnik11 is a code owner automatically assigned from Codecademy/gamut

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ITZSHOAIB @CLAassistant @codecademydev @dreamwasp @smithgajjar09