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: Browser Engine waits for the css selector in Friendly Name wrapped in brackets, if specified #5236

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robert-cardillo
Copy link

No description provided.

@robert-cardillo robert-cardillo force-pushed the real-browser-monitor-wait-for-selector branch from bf0c4db to ffbd312 Compare October 23, 2024 09:58
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

First off all: The PR template is there for a reason, please fill it out.

The name being used like this is reaaaly obvious for users.
A user would have to read this code to figure out why this is happening.
Also using brackets is somewhat common practice in names and not common in specifying a CSS selector (at least as presented)

What would be possible is doing two separate PRs:

  • migrating this monitor to be monitor condition based (see the dns monitor)
  • add a condition to wait for a selector

@CommanderStorm CommanderStorm marked this pull request as draft October 23, 2024 23:39
@CommanderStorm CommanderStorm added area:monitor Everything related to monitors type:enhance-existing feature wants to enhance existing monitor pr:please address review comments this PR needs a bit more work to be mergable labels Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors pr:please address review comments this PR needs a bit more work to be mergable type:enhance-existing feature wants to enhance existing monitor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants