-
Notifications
You must be signed in to change notification settings - Fork 137
syncing used instead of saving while logged in #601
syncing used instead of saving while logged in #601
Conversation
locales/en-US/notes.properties
Outdated
| syncError=Sync Error | ||
|
|
||
| savingChanges=Saving changes… | ||
| savingChanges=Syncing… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change a string you also need to change the associated ID
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings
Also make sure to update the reference in comments (if available)
|
This is not the proper way of fixing the issue. You can't change the property file like that. |
|
@Natim , I was about to fix this. Could you reopen this PR ? I'll try this another way. Or do you want me to file a new PR |
a947815 to
f14daef
Compare
|
This is with respect to #597 (comment)
I tried searching for the word "Syncing" but I found out "Syncing changes". Thus I have used this as you have told me that I can't edit the property file like that.
Same for this I couldn't find "Last Synced" so I did it with "Synced at" instead.
So do you want me to remove this. For now I have removed this , if you want it back then I'll fix this back too 😃 If this is not what you want, then could you explain me a bit more 😃 |
|
@Natim , any views on the recent changes ? |
|
The problem is that now you don't have the saving changes when we are not logged in have you? @ryanfeeley can you confirm this is correct according to the new specs? When not logged in
When sync activated
|
|
@himanish-star You can use the |
|
If we need to go back to Synced 7:24 PM it is a regression of #467 and @ryanfeeley @vladikoff and @flodolo need to talk to figure out what made that change that I don't remember. |
|
Ok, @Natim , I have started correcting my work |
|
Writting the following made me understand why we added the at inside: @ryanfeeley is that really what we want? |
|
@Natim Yes, an example like Synced 7:24 PM is perfect. Same for Saved 7:24 PM. |
f14daef to
7f1c46d
Compare
|
The above changes are with respect to |
|
@himanish-star Is it putting the new text at the end (where the cursor is) or the beginning? Hard to tell as it's the same text. |
|
Well it's putting the text according to the previous caret position. There is an issue already on this #532 |
Natim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a little refactoring.
src/sidebar/panel.js
Outdated
| syncingInProcess = true; | ||
| } | ||
| if (! waitingToReconnect) { | ||
| if (! waitingToReconnect && isAuthenticated) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you can have a if for the !waitingToReconnect and another one for the isAuthenticated condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just saw this now. Well thanks for changing 😃


fixes : #597
Changes can be seen in the GIF below
