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

Preserve cursor position in editors #3485

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Gazook89
Copy link
Collaborator

This PR modifies the editor so that it preserves the cursor position in each the Brew and Style editors. It saves the locations in localStorage, and loads the related position when switching between editors and across page loads.

One side effect is that it does focus the Brew editor on page load. I don't think that is necessarily the worst thing, but I'm not sure that this change should be doing that. But until there is a more focused effort on how focus moves about the page, I think it's fine to leave as-is.

Another thing I thought about while working on this: it might be nice if when you open an editor (changing view or loading the page), the cursor did one or two stronger flashes so you can more easily spot where your cursor is when you first jump into a view. Not likely going in this PR, or any, but mentioning it in case anyone has further thoughts.

I feel like the commit messages (and expandable descriptions) are clear enough. i did try JSDoc annotations as well.

Setter takes a 'view', grabs the current cursor position, and dumps it into localStorage with a related key.

Getter takes a 'view', loads the related localStorage key, and *if* an editor instance is focused, sets the cursor position in it.  The CM instance must have focus on it already-- related code is in next commit.
When the component is mounted, it focuses the editor and the getter method to set the cursor.  It checks for a CM instance first, to avoid issues with properties editor.

When component is unmounted, it saves the cursor position.
When the view/editor changes (text to style etc), the current view cursor is saved, and the new view cursor is loaded.
@Gazook89 Gazook89 added UI/UX User Interface, user experience P3 - low priority Obscure tweak or fix for a single user 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels May 18, 2024
@5e-Cleric
Copy link
Member

Need a deployment to test

@dbolack-ab
Copy link
Collaborator

One side effect is that it does focus the Brew editor on page load. I don't think that is necessarily the worst thing, but I'm not sure that this change should be doing that. But until there is a more focused effort on how focus moves about the page, I think it's fine to leave as-is.

This comes across as a feature, not a bug, to me.

Another thing I thought about while working on this: it might be nice if when you open an editor (changing view or loading the page), the cursor did one or two stronger flashes so you can more easily spot where your cursor is when you first jump into a view. Not likely going in this PR, or any, but mentioning it in case anyone has further thoughts.

This seems like a good idea for a future PR.

@5e-Cleric
Copy link
Member

Another thing I thought about while working on this: it might be nice if when you open an editor (changing view or loading the page), the cursor did one or two stronger flashes so you can more easily spot where your cursor is when you first jump into a view. Not likely going in this PR, or any, but mentioning it in case anyone has further thoughts.

In CodeMirror, as far as i know, the cursor is an html div element:

image

Therefore it would be fairly simple to add styling to make this possible, added and then removed by js.

@Gazook89
Copy link
Collaborator Author

Yeah, not hard, I was already messing with it but still going to leave it for another PR where I’ll present some options and solicit other ideas.

@5e-Cleric
Copy link
Member

image

Not seeing any localstorage item popping up, neither across loads.

@dbolack-ab
Copy link
Collaborator

Duplidcumenting here from chat:

Seems to work after I had my session loaded. Though I'm not sure if it didn't trigger right initially? I didn't see any local storage added. I switched to styles and it crashed. Reloaded to see if I could duplicate it and it then had the values. Worked fine. Reloaded. Deleted the local storage values, switched panes, crash. Looks like it doesn't gracefully handle the values not being in local storage.

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3485 May 20, 2024 19:51 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 - low priority Obscure tweak or fix for a single user 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed UI/UX User Interface, user experience
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

None yet

4 participants