Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement part of #17712 : Acceptance tests for Exploration Creator Section(CUJ 10). #20203
Implement part of #17712 : Acceptance tests for Exploration Creator Section(CUJ 10). #20203
Changes from 2 commits
60e18a2
35fd534
8acdeb1
e892a74
dc9bdf8
940a919
d129dd7
5679fba
1694b1f
6646a83
87e718a
db62083
7e27a5e
41993cc
ee7a5ac
cc09bd6
5a4b699
b3336ca
92c6bda
6cb4377
96055da
2b0696b
2f3ce5c
33cd472
563badb
2342c99
86f6696
c03be55
ae0dca2
ee3eab4
dbf8ad9
c9b86e2
4b0771d
e99c3a0
421ef21
94d8192
fc23f6d
30809ea
0af42ef
292ab19
09a9265
5a0d3be
b56a41b
50de4ff
3975bc9
82d976a
450220e
ddb3ee7
c01317e
fec2046
a07dd38
e77d313
77c3504
7c5634e
0d20eb7
39dad78
d1117a6
901149e
cf492ed
93da30b
2781f9b
e9986b9
a36bb00
399d31f
98b2932
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain in a comment why this if condition is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot approve a PR that has waitForTimeout in it. Please fix before submitting.
/cc @StephenYu2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanlip I have already raised a PR #20231 and trying to get it merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#20180 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to have waitForNavigation() always wait until the network is idle, by default? (Not sure -- what do you think?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they both have their specific use cases -
Here are some scenarios where waitForNavigation() might be preferred over {waitUntil: 'networkidle0'}:
Quick Navigation: If you want to wait for navigation to complete without waiting for all network activity to stop, waitForNavigation() without specifying {waitUntil: 'networkidle0'} is more appropriate. This is useful for scenarios where you don't need to ensure that all resources are fully loaded before proceeding.
Non-Standard Navigation: There may be cases where navigation doesn't involve standard network requests/responses, such as navigating within a single-page application (SPA) where content is loaded dynamically. In such cases, waiting for network idleness might not accurately reflect when navigation is complete, and waitForNavigation() provides a more general solution.
Specific Timing Requirements: If you have specific timing requirements for waiting for navigation, such as waiting until DOMContentLoaded or load events, you might prefer using waitForNavigation() with the appropriate waitUntil option (e.g., 'domcontentloaded', 'load') rather than relying solely on network idleness.
By defaulting to {waitUntil: 'networkidle0'}, you're essentially making your navigation waits more conservative, ensuring that the page is fully loaded and has minimal network activity before proceeding. While this can be suitable for many scenarios, there may still be cases where you prefer the flexibility or specificity offered by waitForNavigation() without specifying network idleness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks -- I guess we're moving towards an SPA so let's leave it as you have it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you abstract this to a utility function like waitForPageToFullyLoad() or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done