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(CoachmarkOverlayElements): Add next/back callbacks and currentStep properties #6548

Conversation

gcattan
Copy link
Contributor

@gcattan gcattan commented Dec 5, 2024

Closes #5596

This PR:

  • Adds callbacks when clicking on Next and Previous buttons.
  • Allows to set the current progression.

What did you change?

Add three optional properties to CoachmarkOverlayElements:

  • onNextCallback
  • onBackCallback
  • currentStep

How did you test and verify your work?

I modified the story book as follows:

image

Then manually checked that:

  • next and back callbacks correctly logged into the console
  • the progression was correctly set.

Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for ibm-products-web-components ready!

Name Link
🔨 Latest commit bcb73e6
🔍 Latest deploy log https://app.netlify.com/sites/ibm-products-web-components/deploys/675afb88475872000849897d
😎 Deploy Preview https://deploy-preview-6548--ibm-products-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for carbon-for-ibm-products ready!

Name Link
🔨 Latest commit bcb73e6
🔍 Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/675afb882ccd0900081ff07f
😎 Deploy Preview https://deploy-preview-6548--carbon-for-ibm-products.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@gcattan gcattan changed the title Feat/5596 coachmarksoverlay extension feat(CoachmarkOverlayElements): Add next/back callbacks and currentStep properties Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.05%. Comparing base (f432ae4) to head (bcb73e6).
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6548      +/-   ##
==========================================
+ Coverage   79.91%   80.05%   +0.13%     
==========================================
  Files         394      394              
  Lines       12888    12907      +19     
  Branches     4267     4277      +10     
==========================================
+ Hits        10300    10333      +33     
+ Misses       2588     2574      -14     
Components Coverage Δ
ibm-products ∅ <ø> (∅)
ibm-products-web-components ∅ <ø> (∅)

@gcattan gcattan marked this pull request as ready for review December 5, 2024 13:18
@gcattan gcattan requested a review from a team as a code owner December 5, 2024 13:18
@gcattan gcattan requested review from elycheea and AlexanderMelox and removed request for a team December 5, 2024 13:18
@gcattan
Copy link
Contributor Author

gcattan commented Dec 6, 2024

@AlexanderMelox CI looks good. But I still cannot access the logs of percy. Can you help with this issue?

@AlexanderMelox
Copy link
Contributor

@AlexanderMelox CI looks good. But I still cannot access the logs of percy. Can you help with this issue?

I just got access to Percy so i can approve now

@gcattan
Copy link
Contributor Author

gcattan commented Dec 11, 2024

Thanks @AlexanderMelox!

@gcattan
Copy link
Contributor Author

gcattan commented Dec 12, 2024

@AlexanderMelox Following our suggestion and summarizing here ou exchange on slack:

  • I renamed the callbacks to onNext and onBack
  • Added a test for the onNext.

However, testing the onBack is a bit more complicated as it is only showing when scrollPercent > 0, but this value never changes in testing (not sure why though).

Anyway, this helped me fix a bug with the scroll position not being correct (the position of the carrousel was not correctly updated).

@@ -248,6 +275,7 @@ export let CoachmarkOverlayElements = React.forwardRef<
);
scrollRef?.current?.scrollToView?.(targetStep);
setCurrentProgStep(targetStep);
onBack?.();
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned on a slack DM, we will have the c4p team add this one in, its a bit more complicated

@AlexanderMelox
Copy link
Contributor

@AlexanderMelox Following our suggestion and summarizing here ou exchange on slack:

  • I renamed the callbacks to onNext and onBack
  • Added a test for the onNext.

However, testing the onBack is a bit more complicated as it is only showing when scrollPercent > 0, but this value never changes in testing (not sure why though).

Anyway, this helped me fix a bug with the scroll position not being correct (the position of the carrousel was not correctly updated).

Jest doesn't really update css styles in testing so thats why it can be a little tricky to test scrolling and such, but we can handle it from now on

@AlexanderMelox AlexanderMelox added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
@elycheea elycheea added this pull request to the merge queue Dec 12, 2024
Merged via the queue into carbon-design-system:main with commit 36bd4f9 Dec 12, 2024
34 checks passed
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.

[Feature Request]: Open CoachmarkOverlayElements for extension
3 participants