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

Change when formula reference insertion happens #1594

Merged
merged 7 commits into from
Aug 5, 2024

Conversation

HactarCE
Copy link
Collaborator

@HactarCE HactarCE commented Jul 23, 2024

Fixes #1593

This PR changes the behavior that determines whether arrow keys or clicking while typing a formula inserts a reference.

The intended behavior of this PR is that clicking outside a cell or using the up/down arrow keys inserts a reference if a reference is expected at the current cursor position in the formula, based only on the nearest non-whitespace token to the left of the cursor. Additionally, clicking in the formula text box confirms the reference insertion, so that the user can then navigate within the textbox using arrow keys.

There is another way that a user can confirm a reference insertion using just the keyboard, which is spoilered in case you'd like to try to figure it out yourself (to gauge whether it's too non-obvious).

Spoiler warning

Type any character, then delete it. Now you can use left/right arrow keys to navigate within the formula text.

Selecting a cell reference with either the mouse or the keyboard allows the user to click or use the up/down arrow keys to replace it with a new cell reference.

This isn't exactly equivalent to Excel's behavior, but it's quite close and in my opinion this behavior is easier to understand and more useful. (In Excel, it's possible to get into a state where it becomes impossible to insert cell references using the mouse and/or keyboard.)

Copy link

vercel bot commented Jul 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
quadratic ✅ Ready (Inspect) Visit Preview Aug 1, 2024 4:37pm

@cla-bot cla-bot bot added the cla-signed label Jul 23, 2024
@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-1594 July 23, 2024 15:55 Inactive
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.99%. Comparing base (2f4af34) to head (786ec18).
Report is 70 commits behind head on qa.

Additional details and impacted files
@@            Coverage Diff             @@
##               qa    #1594      +/-   ##
==========================================
- Coverage   90.00%   89.99%   -0.01%     
==========================================
  Files         188      188              
  Lines       36256    36256              
==========================================
- Hits        32631    32630       -1     
- Misses       3625     3626       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HactarCE
Copy link
Collaborator Author

The cell selection visuals seem to be broken -- is that new here?
image

Also, typing =SUM and then clicking causes inserting a cell reference, so something's not right with this change ...

@HactarCE HactarCE force-pushed the fix-partial-formula-range-select branch from 2e6b6e2 to 3e5c6ae Compare July 26, 2024 19:52
@cla-bot cla-bot bot added the cla-signed label Jul 26, 2024
@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-1594 July 26, 2024 19:52 Inactive
@HactarCE HactarCE changed the title Fix clicking to add formula reference after space Change when formula reference insertion happens Jul 26, 2024
@quadratichq quadratichq deleted a comment from cla-bot bot Jul 26, 2024
@quadratichq quadratichq deleted a comment from cla-bot bot Jul 26, 2024
@quadratichq quadratichq deleted a comment from cla-bot bot Jul 26, 2024
@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-1594 August 1, 2024 16:23 Inactive
@HactarCE
Copy link
Collaborator Author

HactarCE commented Aug 1, 2024

This does not handle arrow keys + shift or cmd/ctrl the way I expect. IMO they should behave the same as arrow keys with no modifiers.

Copy link
Collaborator

@davidkircos davidkircos left a comment

Choose a reason for hiding this comment

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

Functionality looks good to me.

Waiting for review from Luke to merge.

@davidkircos davidkircos merged commit f43a28b into qa Aug 5, 2024
14 checks passed
@davidkircos davidkircos deleted the fix-partial-formula-range-select branch August 5, 2024 23:40
Copy link

qa-wolf bot commented Aug 5, 2024

E2E tests

QA No blocking bugs

Ran Status Preview Started Run time Est. dev time saved
63 workflows Done (Details) Aug 5, 2024 at 11:55 PM (UTC) 5 minutes ~49.83 hrs
⚠️ 1 failed - your wolf pack is investigating
✅ 14 passed

Preexisting bugs

0 Blocking bugs
If you are aware of any of these bugs, you can set their priority to low and prevent them from causing a run failure.
View all blocking bugs

2 Non-blocking bugs

. . . . . . . .

Re-run your E2E tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secondary Formula arguments can't accept a range
3 participants