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

fix(ui-mode): Watch mode button doesn't show active when test selected #34581

Merged
merged 9 commits into from
Feb 11, 2025

Conversation

GeneratorX16
Copy link
Contributor

@GeneratorX16 GeneratorX16 commented Feb 1, 2025

description: The watch mode button was not showing as highlighted when a test in the tree view was focused and the watch icon was being clicked. Handled this issue for both, light mode and dark mode visibility.
#34434
Light Mode (Selected/Focused):
selected-light
Dark Mode (Selected/Focused):
dark-watch

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@GeneratorX16 GeneratorX16 changed the title fix(ui-mode): Watch mode button doesn't show active when test selected fix(ui-mode): Watch mode button doesn't show active when test selected (BUG #34434) Feb 1, 2025
@GeneratorX16 GeneratorX16 changed the title fix(ui-mode): Watch mode button doesn't show active when test selected (BUG #34434) fix(ui-mode): Watch mode button doesn't show active when test selected Feb 1, 2025
@agg23
Copy link
Contributor

agg23 commented Feb 3, 2025

Thank you for the PR. While this is an improvement, this does not pass contrast accessibility, so it technically has not fixed the problem.

Instead of attempting to find a color that works, the correct solution is to draw a box around the active watch icon and keep the watch icon as always white. This isn't the best example screenshot, but you can see how the button is handled.

Screenshot 2025-02-03 at 11 43 53 AM

This comment has been minimized.

@GeneratorX16
Copy link
Contributor Author

GeneratorX16 commented Feb 8, 2025

I have updated the selected icon with the following way:
Dark Mode Hilighted and Selected:
dark-highlight-watch
Dark Mode Highlighted but not selected:
dark-hilight-no-watch

Light Mode Highlighted and Selected:
light-mode-highlight-watch
Light Mode Highlighted but not selected:
light-mode-hilight-no-watch

This comment has been minimized.

description: The watch mode button was not showing as highlighted when a test in the tree view was focused and the watch icon was being clicked. Handled this issue for both, light moide and dark mode visibility

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@agg23 agg23 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. This is a definite improvement. There is more that I'd like to do here, but this fixes an important issue, so it is fine for now.

This comment has been minimized.

@agg23 agg23 merged commit d2e7e8a into microsoft:main Feb 11, 2025
29 checks passed
Copy link
Contributor

Test results for "tests 1"

10 flaky ⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:398:5 › run-server › should terminate network waiters @chromium-ubuntu-22.04-node20
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [installation tests] › tests/playwright-electron-should-work.spec.ts:21:5 › electron should work @package-installations-windows-latest
⚠️ [webkit-library] › tests/library/browsercontext-reuse.spec.ts:50:1 › should reset serviceworker @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:238:3 › should use socks proxy in second page @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/signals.spec.ts:78:7 › signals › should close the browser on SIGINT @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/trace-viewer.spec.ts:109:1 › should show tracing.group in the action list with location @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:82:5 › click should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-set-input-files.spec.ts:245:3 › should upload large file with relative path @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

38499 passed, 797 skipped
✔️✔️✔️

Merge workflow run.

@GeneratorX16 GeneratorX16 deleted the watch-icon-focused branch February 12, 2025 16:00
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.

2 participants