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

Add Confirm Dialog component #5361

Merged
merged 15 commits into from Jul 21, 2022
Merged

Add Confirm Dialog component #5361

merged 15 commits into from Jul 21, 2022

Conversation

fjorgemota
Copy link
Member

@fjorgemota fjorgemota commented Jul 15, 2022

Changes proposed in this Pull Request

  • Adds a generic Confirm Dialog component, heavily inspired by Gutenberg's experimental ConfirmDialog component;
  • Add a hook that allows us to use the Confirm Dialog component in a way similar to the confirm() native function;

Testing instructions

Check "Testing Instructions" on the PR 1537-gh-Automattic/sensei-pro

@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #5361 (13ac0cb) into trunk (9bcfbc3) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              trunk    #5361   +/-   ##
=========================================
  Coverage     40.81%   40.81%           
- Complexity     8638     8639    +1     
=========================================
  Files           285      285           
  Lines         29119    29131   +12     
=========================================
+ Hits          11885    11891    +6     
- Misses        17234    17240    +6     
Impacted Files Coverage Δ
includes/class-sensei-question.php 12.24% <0.00%> (ø)
includes/blocks/class-sensei-blocks.php 21.87% <0.00%> (-2.27%) ⬇️
...ss-sensei-reports-overview-list-table-abstract.php 69.56% <0.00%> (-0.81%) ⬇️
...sensei-reports-overview-data-provider-students.php 90.90% <0.00%> (+0.74%) ⬆️
...ourse-video/class-sensei-course-video-settings.php 26.53% <0.00%> (+1.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e919863...13ac0cb. Read the comment docs.

@fjorgemota fjorgemota requested a review from a team July 15, 2022 22:20
@fjorgemota fjorgemota marked this pull request as ready for review July 15, 2022 22:20
Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

Great PR! I just added some small suggestions.

onConfirm,
onCancel,
} ) => {
useConfirmOnEnter( isOpen, onConfirm );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice code separation here! 😀

*/
import ConfirmDialog from './confirm-dialog';

describe( '<ConfirmDialog />', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great tests!

renatho
renatho previously approved these changes Jul 19, 2022
Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

The changes look great to me!

I just added a small comment about the hook, but I'm already approving it in case you don't agree.

About the styles, maybe we could merge it and tweak the styles in another PR. Unless you really prefer to wait and fix the styles before merging it. 😉

Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

🚀

@fjorgemota fjorgemota merged commit d9312e7 into trunk Jul 21, 2022
@fjorgemota fjorgemota deleted the add/confirm-dialog branch July 21, 2022 15:45
@gikaragia gikaragia added this to the 4.6.0 milestone Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants