add cancel button to undo unwanted save session #512

abaevbog · 2024-09-18T06:26:58Z

Added cancel button to undo saving of items in case it was started by mistake
If none of /connector/save* endpoints were called yet, the button will just exit at saveItems.
If the connector server has been engaged, hit /connector/cancel to tell the server to undo this session
After cancel button is clicked, the connector window is hidden
Minor style tweaks to fit the cancel buttons on the same row as tags input and done button

Depends on: zotero/zotero#4694 that adds the /connector/cancel endpoint

Addresses: #301

A rough demo with the current state of /connector/cancel endpoint.

connector_cancel.mov

- Added cancel button to undo saving of items in case it was started by mistake - If none of /connector/save* endpoints were called yet, the button will just exit at saveItems. - If the connector server has been engaged, hit /connector/cancel to tell the server to undo this session - After cancel button is clicked, the connector window is hidden - Minor style tweaks to fit the cancel buttons on the same row as tags input and done button

abaevbog · 2024-09-18T06:33:35Z

@adomasven I wanted to have a look at one of the connector issues while a number of accessibility changes is being reviewed and this seemed like a good one. Let me know what you think! This ended up being spread across two PRs - I marked both of them as drafts to begin with to make sure it's a reasonably direction to move in.

adomasven · 2024-09-24T06:19:38Z

Yeah, this is good. In the video the Done button seems to be bigger height than the other buttons and there's too much empty space where the Cancel button expands, so let's fix that and go with it.

- place the Done and Cancel buttons in the header - minor re-styling of all buttons in the header so they look more uniform - added new svg-s for relevant buttons, deleted unused files - disclosure button will flip the up/down on each click - cancel and done button will expand on hover and change the icon for textual label - made the entire dialog a bit wider - do not set explicit height of the dialog using scrollHeight anymore, just let it expand as much as it needs unless it leaves outside of the bounds of the screen - made the dialog a flexbox so that the progress box with items can expand as much as it needs and then become scrollable if we run out of space

abaevbog · 2024-09-24T06:54:17Z

Yeah, the border of the button was of the wrong color and I did make the dialog a bit too wide... There were a few other smaller things with spacing that I tweaked. How is it looking now?

Screen.Recording.2024-09-23.at.11.44.45.PM.mov

And I just cleaned up earlier unrelated changes and pushed these changes. One question: I added new svg files to use for done, cancel and the disclosure button - it does look like in the build process, there is a step to fetch relevant images from the main repo but the submodule points at a pre-redesign commit that doesn't have all the new svg files, so I assumed just adding new files like this is they way to go for now?

Also, I was doing all the testing on firefox, so let me make sure everything looks right on chrome and safari.

abaevbog · 2024-09-24T07:19:18Z

Chrome looks good to me now as well. Per https://github.com/zotero/safari-app-extension, do I need to be a part of the team to build and sign the safari extension? Sorry if that's an obvious question, it's been a while since I had to work with xcode.

dstillman · 2024-09-24T08:18:44Z

Don't bother trying to test on Safari for now — it's too finicky. We'll check and report back.

adomasven · 2024-09-24T08:39:49Z

One question: I added new svg files to use for done, cancel and the disclosure button - it does look like in the build process, there is a step to fetch relevant images from the main repo but the submodule points at a pre-redesign commit that doesn't have all the new svg files, so I assumed just adding new files like this is they way to go for now?

That's fine. There's a PR pending with updated icons (with the zotero submodule updated) that I am waiting an OK from Dan, but it shouldn't hold this up.

and no margin after the last visible button in header

abaevbog · 2024-09-24T17:05:46Z

Sounds good! Meanwhile, I added some final touches to remove unwanted spacing after buttons and to not show the "done" button unless target selector is expanded per #512 (comment). I unintentionally reverted that change before pushing yesterday. With these final tweaks, I think I can un-draft this.

adomasven · 2024-09-26T06:20:25Z

@abaevbog You should test the manifestv3 build on chrome. I'll remove the chrome build as it's now fully phased out. EDIT: Actually it's still used by Edge, so keeping it for now.

adomasven · 2024-09-26T06:22:19Z

src/common/progressWindow/progressWindow.css

+	/* Make progress box scrollable if there are too many items */
+	display: flex;
+	flex-direction: column;
+	overflow-y: scroll;


This adds a permanent scrollbar on non-macOS even when there is nothing to scroll. Should be overflow-y: auto

adomasven · 2024-09-26T06:32:30Z

@abaevbog One more thing. We should enable this with a Zotero feature check. The response to /ping on Zotero should include supportsSaveCancelling and Connector should only show the cancel button in that case. Also no cancel button if saving to server.

Do not show cancel button if the client did not return supportsSaveCancelling from /ping or if we are saving to web library.

abaevbog · 2024-09-26T20:10:15Z

You should test the manifestv3 build on chrome.

Gotcha, so from now on, I'll use manifestv3 build folder for chrome but still firefox for firefox.

One more thing. We should enable this with a Zotero feature check.

Oh, that is a great point! Otherwise, one could have an updated connector that would try to hit /connector/cancel that the client might not yet handle.

I pushed to the main repo PR to return supportsSaveCancelling: true together with other prefs when /connector/ping is hit. And here, we just check if it was received and hide the button if it wasn't OR if we're saving to web library. So, for instance, there iw not X button if I run it with the current beta.

adomasven · 2024-09-27T06:04:37Z

src/common/ui/ProgressWindow.jsx

@@ -127,7 +127,7 @@ Zotero.UI.ProgressWindow = class ProgressWindow extends React.PureComponent {

 		// Check if the X button to cancel a saving process should be displayed
 		(async () => {
-			let notSavingToWebLibrary = await Zotero.Connector.getClientVersion();
+			let notSavingToWebLibrary = await Zotero.Connector.checkIsOnline();


This issues a superfluous /ping call to the client. The ProgressWindow should already know whether it's saving to Zotero or the server.

It looks like we always try to get available collections from the client (even if there is no zotero client running) and pass the targets (which are empty in case of saving to web) to changeHeadline which updates the state of ProgressWindow. And then, this.state.targets is used to determine if the headline select for collections should be rendered. Isthis.state.targets a reasonable way to determine if we are saving to desktop vs web? It does seems to work for instances that I tried, I just wasn't sure if there are times when it would not be the case.

I think it's just this.state.targetSelectorShown? But target selector is written by Dan.

targetSelectorShown is updated each time when the chevron is clicked, so I think it is about whether the target selector table and tags region is expanded or collapsed...

But judging by this comment as well, does it seem like the existence of targets should imply that we fetched them from the client, while their non-existence should imply that we'll save to the server? I feel like it does... Combined with the fact that in updateFromClient, "Saving to zotero.org" is set without any targets and "Saving to ..." is set with the targets, I am somewhat convinced that this.state.targets == saving to the desktop client.

@dstillman, correct me if I am wrong on the above though!

Instead of an extra checkOnline call

Fixed bug where an error would be thrown in the end of changeHeadline if there is no target and the header would not get set.

This comment was marked as resolved.

Sign in to view

fill=black for chevron

3dc4551

no done btn unless target selector expanded

3168257

and no margin after the last visible button in header

abaevbog marked this pull request as ready for review September 24, 2024 17:05

Fix cancel button changes breaking MV3

4a537eb

adomasven reviewed Sep 26, 2024

View reviewed changes

Fix progress window always showing a scrolbar

6f6eb01

abaevbog added 2 commits September 26, 2024 12:57

only show cancel button if the client supports it

f27f4a2

Do not show cancel button if the client did not return supportsSaveCancelling from /ping or if we are saving to web library.

getClientVersion -> checkIsOnline

f7a9859

adomasven reviewed Sep 27, 2024

View reviewed changes

abaevbog added 2 commits September 26, 2024 23:40

this.state.targets to determine if saving to web

3f7137a

Instead of an extra checkOnline call

fix "Saving to zotero.org" not appearing in header

f78fa67

Fixed bug where an error would be thrown in the end of changeHeadline if there is no target and the header would not get set.

add cancel button to undo unwanted save session #512

Are you sure you want to change the base?

add cancel button to undo unwanted save session #512

Conversation

abaevbog commented Sep 18, 2024

abaevbog commented Sep 18, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

adomasven commented Sep 24, 2024

abaevbog commented Sep 24, 2024

abaevbog commented Sep 24, 2024

dstillman commented Sep 24, 2024

adomasven commented Sep 24, 2024

abaevbog commented Sep 24, 2024

adomasven commented Sep 26, 2024 • edited Loading

adomasven Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

adomasven commented Sep 26, 2024

abaevbog commented Sep 26, 2024 • edited Loading

adomasven Sep 27, 2024

Choose a reason for hiding this comment

abaevbog Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

adomasven Sep 27, 2024

Choose a reason for hiding this comment

abaevbog Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

abaevbog Sep 27, 2024

Choose a reason for hiding this comment

adomasven commented Sep 26, 2024 •

edited

Loading

adomasven Sep 26, 2024 •

edited

Loading

abaevbog commented Sep 26, 2024 •

edited

Loading

abaevbog Sep 27, 2024 •

edited

Loading

abaevbog Sep 27, 2024 •

edited

Loading