Skip to content

Conversation

@cpAdm
Copy link
Contributor

@cpAdm cpAdm commented Mar 28, 2025

Implements points:

  1. Keep my query when clicking project filter
  2. Ctrl + click should also work with project labels (and not open new tab)

Note: The full query now gets properly escaped, this does mean that the URL looks different. However, old URLs still work.

References: #35212

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Test results for "tests 1"

2 failed
❌ [webkit-library] › tests/library/screenshot.spec.ts:205:14 › element screenshot › element screenshot should work with a mobile viewport @webkit-ubuntu-22.04-node18
❌ [webkit-page] › tests/page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18

4 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:424:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-pages.spec.ts:82:3 › should click the button with offset with page scale @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/trace-viewer.spec.ts:1306:1 › should pick locator in iframe @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

38933 passed, 805 skipped
✔️✔️✔️

Merge workflow run.

@cpAdm
Copy link
Contributor Author

cpAdm commented Mar 28, 2025

Failed tests seem flaky to me

Copy link
Member

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

The change looks good. Please try reverting the unrelated newline changes where possible.

@cpAdm cpAdm requested a review from Skn0tt March 31, 2025 12:57
Skn0tt
Skn0tt previously approved these changes Mar 31, 2025
Copy link
Member

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

cc @dgozman for final review

@github-actions

This comment has been minimized.

cpAdm added 2 commits March 31, 2025 15:53
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Test results for "tests 1"

4 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:424:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [chromium] › tests/headerView.spec.tsx:46:1 › should toggle filters @web-components-html-reporter
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

38935 passed, 805 skipped
✔️✔️✔️

Merge workflow run.

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! There is one small caveat I'd like to resolve, but otherwise looks awesome.

projectName: string,
}> = ({ projectNames, projectName }) => {
const encoded = encodeURIComponent(projectName);
const value = projectName === encoded ? projectName : `"${encoded.replace(/%22/g, '%5C%22')}"`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this logic was here to support project names with spaces/quotes inside. Otherwise, tokenizing p:my project will yield two tokens instead of a single one with "my project" as a project name. I guess we should add a test as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Should I make it generic such that it also works for annotations and such?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would be great.

header={<span>
{file.fileName}
</span>}>
header={<span>{file.fileName}</span>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like unrelated formatting change?

@cpAdm
Copy link
Contributor Author

cpAdm commented Apr 21, 2025

I won't have the time to complete this. So closing.

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.

3 participants