Skip to content

Conversation

@DobroTora
Copy link
Contributor

@DobroTora DobroTora commented Sep 9, 2025

Proposed behaviour

Adding play functions to the Dialog component.

Current behaviour

There are no play functions in this component currently.

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

Copy link
Contributor

@damienrobson-sage damienrobson-sage left a comment

Choose a reason for hiding this comment

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

In addition to Ed's comments, it's worth considering that using userInteractionPause to simulate very short amounts of delay goes against the reason for it being used in the first place. For example, userInteractionPause(50) would simulate a user "waiting" for 0.05s which is near instantaneous.

Whilst we should be avoiding using it in certain situations, if it is needed the length of time should be more realistic (e.g. to simulate a user waiting a few seconds for a modal to open and any associated animation to complete)

@DobroTora
Copy link
Contributor Author

In addition to Ed's comments, it's worth considering that using userInteractionPause to simulate very short amounts of delay goes against the reason for it being used in the first place. For example, userInteractionPause(50) would simulate a user "waiting" for 0.05s which is near instantaneous.

Whilst we should be avoiding using it in certain situations, if it is needed the length of time should be more realistic (e.g. to simulate a user waiting a few seconds for a modal to open and any associated animation to complete)

I agree with Damo when it comes to using pauses when we want to simulate user's behavior and I think it came out also in one of the previous reviews.

@damienrobson-sage
Copy link
Contributor

I agree with Damo when it comes to using pauses when we want to simulate user's behavior and I think it came out also in one of the previous reviews.

Apologies Debra, looking back I think I've badly-worded my original comment (or ended it without making any actual point). What I meant was that, in 90% of cases, a simple await expect callback will suffice. In these tests, we don't need to simulate users waiting for anything in particular. Maybe if we were simulating an API call or something of that ilk, but here we're just waiting for animations to complete and the DOM to stabilise, so we can just use the de-facto methods.

Copy link
Contributor

@edleeks87 edleeks87 left a comment

Choose a reason for hiding this comment

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

I'm confused why some AnchorNav changes are in this PR but I've assumed it was an accident and that it was meant for a separate one so have added comments to these files to help etc

@DobroTora
Copy link
Contributor Author

DobroTora commented Sep 22, 2025

I agree with Damo when it comes to using pauses when we want to simulate user's behavior and I think it came out also in one of the previous reviews.

Apologies Debra, looking back I think I've badly-worded my original comment (or ended it without making any actual point). What I meant was that, in 90% of cases, a simple await expect callback will suffice. In these tests, we don't need to simulate users waiting for anything in particular. Maybe if we were simulating an API call or something of that ilk, but here we're just waiting for animations to complete and the DOM to stabilise, so we can just use the de-facto methods.

Ok, I think I misunderstood the idea as well then, as I remember we discuss it before and agree on using this method over toBeVisible. I am following the comments now and took a note of the method preference for a future.

@edleeks87 edleeks87 marked this pull request as ready for review September 29, 2025 15:00
@edleeks87 edleeks87 marked this pull request as draft September 30, 2025 08:30
@DobroTora DobroTora marked this pull request as ready for review October 1, 2025 08:38
@DobroTora DobroTora marked this pull request as draft October 1, 2025 11:04
@DobroTora DobroTora marked this pull request as ready for review October 6, 2025 14:34
@DobroTora DobroTora marked this pull request as draft October 6, 2025 16:32
Copy link
Contributor

@edleeks87 edleeks87 left a comment

Choose a reason for hiding this comment

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

There's not much value in calling focus() and then asserting it's focused, I think we should test the behaviour built into the component where focus is on the wrapper on open

@DobroTora DobroTora marked this pull request as ready for review October 10, 2025 11:30
@DobroTora DobroTora marked this pull request as draft October 10, 2025 11:44
edleeks87
edleeks87 previously approved these changes Oct 13, 2025
@edleeks87 edleeks87 marked this pull request as ready for review October 13, 2025 14:08
@DobroTora DobroTora dismissed stale reviews from edleeks87 and damienrobson-sage via 17918f6 October 13, 2025 16:37
@DobroTora DobroTora marked this pull request as draft October 13, 2025 17:14
@DobroTora DobroTora marked this pull request as ready for review October 14, 2025 11:06
edleeks87
edleeks87 previously approved these changes Oct 15, 2025
@DobroTora DobroTora dismissed stale reviews from edleeks87 and damienrobson-sage via 0b654c9 October 21, 2025 08:44
@DobroTora DobroTora force-pushed the FE-7237-Dialog-PF branch 2 times, most recently from 0913f63 to cbead49 Compare October 21, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants