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

Fixed: Added a button to close model. #1312

Merged
merged 6 commits into from
Feb 11, 2025

Conversation

ZendeAditya
Copy link
Contributor

@ZendeAditya ZendeAditya commented Jan 11, 2025

Date: 13/01/2025

Developer Name: Aditya Zende (aditya-zende-1)


Issue Ticket Number

Description

Added a cross button on Task Update Modal.

Documentation Updated?

  • Yes
  • No

Under Feature Flag

  • Yes
  • No

Database Changes

  • Yes
  • No

Breaking Changes

  • Yes
  • No

Development Tested?

  • Yes
  • No

Screenshots

Screenshot 1

Added icon only instead of button

loom-video.2.mp4

Test Coverage

Screenshot 1

Test Coverage Screenshot]

Additional Notes

Copy link

vercel bot commented Jan 11, 2025

@ZendeAditya is attempting to deploy a commit to the RDS-Team Team on Vercel.

A member of the Team first needs to authorize it.

@shobhan-sundar-goutam
Copy link

  1. Why package.json is committed?

  2. This part is not correct. Please check in other PRs on how to do it correctly.
    image

  3. Do you already have any design for the close button? If not, I would suggest you to check other modals of RDS for the close button design.

Copy link
Member

@AnujChhikara AnujChhikara left a comment

Choose a reason for hiding this comment

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

Build is failing, likely due to the Node.js version upgrade. Please check

@tejaskh3
Copy link
Member

The build is failing, please fix that.

@tejaskh3
Copy link
Member

Date: 11/01/2025

Developer Name: Aditya Zende (aditya-zende-1)

Issue Ticket Number

#1313

Description

In the reusable main Model component, I added a close button with an onclick event handler and a code to close the model with an esc button.

loom-video.4.mp4

Documentation Updated?

  • Yes
  • [ ✅] No

Under Feature Flag

  • Yes
  • [ ✅] No

Database Changes

  • Yes
  • [ ✅] No

Breaking Changes

  • Yes
  • [ ✅] No

Development Tested?

  • Yes
  • [ ✅] No

Screenshots

Screenshot 1

Test Coverage

Screenshot 1

Additional Notes

Why is development tested no here?

package.json Outdated Show resolved Hide resolved
src/components/Modal/index.tsx Outdated Show resolved Hide resolved
src/components/Modal/modal.module.scss Outdated Show resolved Hide resolved
@ZendeAditya
Copy link
Contributor Author

Hey @tejaskh3 @AnujChhikara Please check it now i fix the node version back to 18

@AnujChhikara
Copy link
Member

Hey @tejaskh3 @AnujChhikara Please check it now i fix the node version back to 18

where? can you please check if you have pushed latest changes.

@ZendeAditya
Copy link
Contributor Author

Hey @tejaskh3 @AnujChhikara Please check it now i fix the node version back to 18

where? can you please check if you have pushed latest changes.

image
I restored it back

@tejaskh3
Copy link
Member

as the implementation is changed, can you please update the video or screenshot?

@ZendeAditya
Copy link
Contributor Author

as the implementation is changed, can you please update the video or screenshot?

Yes I will update it

@ZendeAditya
Copy link
Contributor Author

ZendeAditya commented Jan 13, 2025

@tejaskh3 updated image and video with a new one

Copy link
Member

@pankajjs pankajjs left a comment

Choose a reason for hiding this comment

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

Left comments

src/components/Modal/index.tsx Outdated Show resolved Hide resolved
src/components/Modal/index.tsx Outdated Show resolved Hide resolved
@ZendeAditya
Copy link
Contributor Author

ZendeAditya commented Jan 15, 2025

Can you please check this @tejaskh3 @pankajjs

@ZendeAditya ZendeAditya requested a review from pankajjs January 15, 2025 13:29
@AnujChhikara
Copy link
Member

Hello @ZendeAditya can you please write test for you changes and attach test coverage in the description

@ZendeAditya
Copy link
Contributor Author

Hello @ZendeAditya can you please write test for you changes and attach test coverage in the description

Yes

Copy link
Member

@tejaskh3 tejaskh3 left a comment

Choose a reason for hiding this comment

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

Instead the icon you used,

Please use the icon that is shown in the screenshot, if it is not their please get that from dashboard site.
image

@ZendeAditya
Copy link
Contributor Author

Instead the icon you used,

Please use the icon that is shown in the screenshot, if it is not their please get that from dashboard site.
image

Ok I will add it

@ZendeAditya
Copy link
Contributor Author

@tejaskh3 @AnujChhikara
I have added the icon according to the screenshot and also wrote a test case for my code. please review again.

@ZendeAditya ZendeAditya requested a review from tejaskh3 January 17, 2025 15:14
@ZendeAditya
Copy link
Contributor Author

@tejaskh3 @AnujChhikara I have added the icon according to the screenshot and also wrote a test case for my code. please review again.

please check this

@pankajjs
Copy link
Member

Please make your changes behind feature flag.
Thank you.

src/components/taskDetails/TaskUpdateModal.tsx Outdated Show resolved Hide resolved
src/components/Modal/index.tsx Outdated Show resolved Hide resolved
@tejaskh3 tejaskh3 self-requested a review January 21, 2025 04:55
@VinuB-Dev
Copy link
Contributor

@ZendeAditya This comment #1312 (comment) is not yet resolved

@ZendeAditya ZendeAditya requested a review from VinuB-Dev February 9, 2025 05:49
@ZendeAditya ZendeAditya requested a review from VinuB-Dev February 9, 2025 08:50
@ZendeAditya ZendeAditya requested a review from VinuB-Dev February 9, 2025 10:24
@VinuB-Dev
Copy link
Contributor

Code looks good.
Can you please add a video / gif of working solution also. Show both cases i.e. with dev flag and without it. Add it under the screenshots in PR description. The current code video is not required.
Please request a member to run the test workflow also.

@ZendeAditya
Copy link
Contributor Author

Hey @tejaskh3 @AnujChhikara what Is the next process for merging PR.

@AnujChhikara
Copy link
Member

Hey @tejaskh3 @AnujChhikara what Is the next process for merging PR.

you can tag Tejas sir and asked him to merge this PR and mention you got 3 approvals

@iamitprakash iamitprakash merged commit f8c18bc into Real-Dev-Squad:develop Feb 11, 2025
1 of 2 checks passed
@pankajjs pankajjs mentioned this pull request Feb 11, 2025
10 tasks
iamitprakash added a commit that referenced this pull request Feb 12, 2025
* Add modal to acknowledge user about status change on marking progress to 100% (#1322)

* initial commit

* refactor code

* added feature flag

* refactored some code

* changed casing and removed comments

* changed to predefined colouring

* removed unnecessary useCallbacks

* removed onstatus function

* resolved commented changes

* decereased the mismatch between the modal and design doc modal

* changed button color

* font change

* used the already created modal component

* dimensional changes

* refactored function

* small refactoring

* async await updated

* applied callback on close icon

* refactored to named exports

* removed a div element

* rounded off the rem values

* put the close icon inside button wrapper

* rounded off rems

* rounded off rem values

* Fixed: Added a button to close model. (#1312)

* Close button added

* Added cross button on update modal

* Formatting done

* All Test Passed

* Unnecessery Code Removed

* Fix test to check if setIsOpen is called on close button click

* Tests / Add modal to acknowledge user about status change on marking progress to 100% (#1324)

* initial changes

* removed unnecessary lines

* removed the onStatusChange tests

* added one more check to ensure that the onclose() function is working

* added more tests

* added should before each line

* added braces

* added dev flag tests

* added the testid check

* added skip to tests

* added skeleton components to pass linitng checks

* added one more test case

* removed skips

* added a getAllByRole

---------

Co-authored-by: Rishi Chaubey <155433512+RishiChaubey31@users.noreply.github.com>
Co-authored-by: Aditya Zende <adityazende6710@gmail.com>
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.

[FIX]: modal on the status site to update the task
8 participants