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

Live Mirror Editor position in Preview pane. #3484

Merged
merged 57 commits into from
Sep 16, 2024

Conversation

dbolack-ab
Copy link
Collaborator

@dbolack-ab dbolack-ab commented May 18, 2024

Extends #3483 by adding live scroll following the editor window.

Toggled with Lock icon on center pane. Defaults to locked.

Catches mouseclicks, CTRL-HOME, CTRL-END. Also triggers on arrow keys and enter.

Done as a separate PR in case this is a really crap solution but 3483 isn't.

Resolves part 2 of #241

To Verify:

  • Verify "Scroll Lock" Toggle changes state and behavior
  • Verify "Scroll Lock" behavior persists after closing and reopening Homebrewery. ( Load a page, toggle the lock, close the window, reopen Homebrewery. )

This catchs CTRL-HOME, CTRL-END, and mouseclicks.

It tests for changes on arrow keys and enter.

Not sure if it's the best way to do this, but it works quickly on a large, crappy brew.
@dbolack-ab dbolack-ab self-assigned this May 18, 2024
@5e-Cleric
Copy link
Member

I believe a checkbox in the snippet bar would be in order. Might be time to have some settings in the editor pane.

@calculuschild
Copy link
Member

calculuschild commented May 18, 2024

I think it would be more clear to just add a third button alongside the other two "sync" buttons on the divider bar. Keep all the "sync" buttons near each other.

The original issue reporter suggested a "lock" icon.

@dbolack-ab
Copy link
Collaborator Author

I think it would be more clear to just add a third button alongside the other two "sync" buttons on the divider bar. Keep all the "sync" buttons near each other.

The original issue reporter suggested a "lock" icon.

Try not to laugh but I did not realise that's what those icons were. :)

The button displays the *next* state of the toggle. IE, if the current state is locked ( Live scrolling is active ) the icon is unlock with a corresponding mouse-over.

It may be desirable to invert this.
@5e-Cleric 5e-Cleric added the 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed label May 19, 2024
@dbolack-ab
Copy link
Collaborator Author

Updated to match parent and switch hotkey to scrolllock because. Needs platform testing.

@calculuschild
Copy link
Member

If you click the lock button on one brew and then open a new brew, the behavior remains but the button shows unlocked. Button needs to load the correct icon based on the currently-selected option when a page is opened..

@calculuschild
Copy link
Member

calculuschild commented May 19, 2024

I'm not able to get actual scroll sync to work, it appears to only sync based on cursor position in the text editor.

For example, when I scroll up and down on the brew preview side, it does not scroll the editor into the same page and vice/versa. Not sure if I am missing something but I haven't had time to look at the code yet to check where the bug might be.

@dbolack-ab
Copy link
Collaborator Author

For example, when I scroll up and down on the brew preview side, it does not scroll the editor into the same page and vice/versa. Not sure if I am missing something but I haven't had time to look at the code yet to check where the bug might be.

Ah. You know, that's a mode I hadn't thought of. I only ran it one way - based on editor position. Let me look at that.

@dbolack-ab
Copy link
Collaborator Author

If you click the lock button on one brew and then open a new brew, the behavior remains but the button shows unlocked. Button needs to load the correct icon based on the currently-selected option when a page is opened..

Open a new brew in a new window?

@calculuschild
Copy link
Member

If you click the lock button on one brew and then open a new brew, the behavior remains but the button shows unlocked. Button needs to load the correct icon based on the currently-selected option when a page is opened..

Open a new brew in a new window?

For example if I am in the /new page and start working on a brew and click the lock, then I save it so it opens an /edit page. The lock behavior remains but the button doesn't match the actual state.

@Gazook89
Copy link
Collaborator

Quick thoughts:

A. I wonder if the animated scrolling is motion sickness inducing. I never thought about that with the existing two 'sync' buttons, but that is probably because those are quick, explicit triggers that happen once. This 'link' button means that animations are triggering a lot more--- and with a less explicit trigger. I don't know if this concern is justified. If it is, I'm not sure if the best solution is to get rid of the animation and just snap to the correct page? Or have that tied to a User Account setting ('prefers reduced motion') or to the browser/user-agent preferences via CSS (@media (prefers-reduced-motion)).

B. I know this was asked in gitter and is just a preference thing, but I think the "lock" should show "locked" when it is currently locked, and not the "future state". Maybe another option to consider is to change the icon onHover.

C. This isn't part of your PR and doesn't need to be, but I have long thought those snippet bar buttons are odd looking-- they are the only rounded element in all of Homebrewery, the only ones with borders, the only ones with shadows (besides pages) and their font-size is larger than any other UI label when in fact they are the least important item on the screen. Here is a simple vision:

image

This reduces the overlap clutter, reduces the visual dominance, increases cohesiveness, and gives us a starting point for more "preview pane" controls. Most importantly for this PR, the button also has a visual cue that it is toggled-- the darkened background color. This matches the behavior of the "current editor" toggle just to the left of it.

@calculuschild
Copy link
Member

I would recommend we leave any styling changes for a separate PR (where everyone can debate as much as they like) and just let this one focus on getting the function.

@dbolack-ab
Copy link
Collaborator Author

dbolack-ab commented May 22, 2024

I would recommend we leave any styling changes for a separate PR (where everyone can debate as much as they like) and just let this one focus on getting the function.

👍

@Gazook89
Copy link
Collaborator

Gazook89 commented May 22, 2024

For example if I am in the /new page and start working on a brew and click the lock, then I save it so it opens an /edit page. The lock behavior remains but the button doesn't match the actual state.

I think the current setting just needs to be saved in localStorage, and loaded when the component mounts. This is assuming that the setting should persist across brews and loads (which i think it should).

Edit: maybe sessionStorage is better here-- this way if you have multiple tabs open, the same setting isn't applied across all of them.

@dbolack-ab
Copy link
Collaborator Author

For example if I am in the /new page and start working on a brew and click the lock, then I save it so it opens an /edit page. The lock behavior remains but the button doesn't match the actual state.

I think the current setting just needs to be saved in localStorage, and loaded when the component mounts. This is assuming that the setting should persist across brews and loads (which i think it should).

Edit: maybe sessionStorage is better here-- this way if you have multiple tabs open, the same setting isn't applied across all of them.

I wonder if this should be a document setting or a client setting....

@5e-Cleric
Copy link
Member

5e-Cleric commented Jun 1, 2024

I wonder if this should be a document setting or a client setting....

If it was me, this would be a user setting, but set in localStorage instead of DB, for an easier implementation. So save the state of the lock in localstorage and get it when mounting the component.

Edit: maybe sessionStorage is better here-- this way if you have multiple tabs open, the same setting isn't applied across all of them.

As long as they don't reload manually the page i don't think the re-render would update the lock, would it?

@Gazook89
Copy link
Collaborator

Gazook89 commented Jun 1, 2024

The scroll-lock should be saved per client, not per brew. If you open a few brews, it could be very weird to have the behavior of your editor change with each.

If you use sessionStorage, you could still have a few brews open in different tabs and use different scroll-lock behavior in each, since sessionStorage isn't shared across tabs/windows.

As long as they don't reload manually the page i don't think the re-render would update the lock, would it?

I'm not sure I understand this question. If the user reloads the page, it keeps the same sessionStorage value. From the MDN article:

A page session lasts as long as the tab or the browser is open, and survives over page reloads and restores.

@5e-Cleric
Copy link
Member

5e-Cleric commented Jun 1, 2024

As long as they don't reload manually the page i don't think the re-render would update the lock, would it?

I'm not sure I understand this question. If the user reloads the page, it keeps the same sessionStorage value. From the MDN article:

What i mean is, if they change the lock state in one tab, the rest of the tabs will be unlocked even if in the same session until they get a manual reload, would they not?

NVM, a change in sessionStorage would trigger a rerender of the state, which should trigger a rerender of the element if set correctly.

@5e-Cleric
Copy link
Member

I'm assuming once this advances enough a deployment will be issued and new reviews requested.

@calculuschild
Copy link
Member

calculuschild commented Sep 13, 2024

I'm assuming once this advances enough a deployment will be issued and new reviews requested.

Yep. Got partway through but haven't quite been able to finish it off yet. Work is taking my focus for a couple days.

@calculuschild
Copy link
Member

calculuschild commented Sep 16, 2024

Going to merge this now. Possibility for slight behavior tweaks in the future but I think this checking all the boxes now.
Thanks @dbolack-ab !

Realized I can't send out a new test deployment since Heroku deletes its knowledge of the whole branch when the PR is closed (the menu for manual deployment can't see this branch anymore). However this will now be integrated into staging https://homebrewery-stage.herokuapp.com/new if people want to fiddle around with it.

@calculuschild calculuschild merged commit a283438 into naturalcrit:master Sep 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🔍 R4 - Reviewed - Fixed! Awaiting final review⭐ PR review comments addressed UI/UX User Interface, user experience
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEATURE]: Scroll Linking
5 participants