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

Cancel gesture diagram #2762

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Howlmaster
Copy link

I fixed the issue : [mobile] Cancel gesture diagram #2738

I took a look at #2745 and I added the cancel gesture to the last the gesture menu.
And now, it meets all expected behaviors.

@Howlmaster Howlmaster marked this pull request as draft January 9, 2025 17:00
Copy link
Contributor

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

Thanks for your submission! Works great. Thanks for covering all the requirements.

Below are the issues I found. Please let me know if you have any questions. Thanks!

src/commands/cancel.ts Outdated Show resolved Hide resolved
src/commands/cancel.ts Outdated Show resolved Hide resolved
src/commands/cancel.ts Outdated Show resolved Hide resolved
src/components/CommandPalette.tsx Outdated Show resolved Hide resolved
@raineorshine
Copy link
Contributor

Current Behavior

When selected in the command palette, "Cancel" is rendered with the foreground color (white).

Paste 2025-01-09 12 26 55

Expected Behavior

The selected command should always be rendered with the highlight color (blue).

@raineorshine
Copy link
Contributor

Current Behavior

The Cancel GestureDiagram is slightly too large and is positioned too close to the Help diagram.

Paste 2025-01-09 12 33 21

Expected Behavior

The Cancel GestureDiagram should be slightly smaller and positioned evenly between the commands that come before and after, just like other commands.

@raineorshine
Copy link
Contributor

raineorshine commented Jan 9, 2025

Steps to Reproduce

  1. Decrease the font size using the font controls in the footer.
  2. Activate the Command Palette by inputing a gesture.

Current Behavior

The Cancel GestureDiagram does not scale with font size.

Paste 2025-01-09 12 25 01

Expected Behavior

The Cancel GestureDiagram should scale with font size like the other GestureDiagrams.

@raineorshine
Copy link
Contributor

raineorshine commented Jan 9, 2025

Current Behavior

The Cancel GestureDiagram is highlighted (blue) even when a valid gesture (New Thought) is selected.

Paste 2025-01-09 12 33 21

Expected Behavior

The Cancel GestureDiagram should be dim (gray) when not selected. It should only be highlighted when selected.

@raineorshine
Copy link
Contributor

raineorshine commented Jan 9, 2025

Steps to Reproduce

yarn test:puppeteer commandPalette

Note that docker is required to run the Puppeteer tests.

Current Behavior

The Puppeteer test are failing because a snapshot is out of date.

commandPalette-diff

You can view the snapshot diff from GitHub Actions in the build artifacts. For more information, see: https://github.com/cybersemics/em/wiki/Testing#5-visual-snapshot-tests

Expected Behavior

The Puppeteer tests should all pass.

To update the snapshots for this test, run:

yarn test:puppeteer commandPalette -u

@raineorshine
Copy link
Contributor

raineorshine commented Jan 9, 2025

Current Behavior

The Cancel diagram does not show up in the snapshot test for https://github.com/cybersemics/em/blob/5bfeca0d0823f3863d68e6b2202ccef6db8251b5/src/components/modals/TestGestureDiagram.tsx.

gesture-diagram-1

Expected Behavior

The Cancel diagram should be added to https://github.com/cybersemics/em/blob/5bfeca0d0823f3863d68e6b2202ccef6db8251b5/src/components/modals/TestGestureDiagram.tsx as requested in the original issue. The snapshot test should be updated once added.

@raineorshine
Copy link
Contributor

@Howlmaster Just checking in! Let me know if you have any questions about the requested changes.

@Howlmaster Howlmaster marked this pull request as ready for review January 16, 2025 11:22
@raineorshine
Copy link
Contributor

Thanks!

Looks like the yarn.lock was inadvertently changed.

@Howlmaster Howlmaster force-pushed the cancel-gesture-diagram branch from 908ee84 to 4c09fab Compare January 16, 2025 15:15
Copy link
Contributor

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

I see that all of the comments were marked resolved from my previous review, yet I don't see any changes in the diff, which makes it look like they were not addressed. I've unresolved them for now. Are you still working on these?

@Howlmaster Howlmaster force-pushed the cancel-gesture-diagram branch from 9f6f125 to a780a7e Compare January 17, 2025 17:18
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