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

New translate #179

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

New translate #179

wants to merge 6 commits into from

Conversation

dima-bzz
Copy link

@dima-bzz dima-bzz commented Apr 8, 2020

Added simple translate

… for guide. Added onCursorActivity for update cursor position.

Signed-off-by: Dmitry Mazurov <dimabzz@gmail.com>
Signed-off-by: Dmitry Mazurov <dimabzz@gmail.com>
Signed-off-by: Dmitry Mazurov <dimabzz@gmail.com>
@dima-bzz
Copy link
Author

@Ionaru What about my PR?

@Ionaru
Copy link
Owner

Ionaru commented Apr 27, 2020

Again, you cannot remove an option, because that is a breaking change.
Can you somehow merge the existing autosave.text into the new system?

@dima-bzz
Copy link
Author

@Ionaru And what exactly will be the problem? I did autosave.text just recently. And this PR will quietly replace autosave.text with the default value.

@Ionaru
Copy link
Owner

Ionaru commented Apr 27, 2020

autosave.text still needs to work like it did before, or it will change (break) existing programs.

@dima-bzz
Copy link
Author

@Ionaru So it will work as before. I only changed the translation, which does not affect the work.

@@ -2025,7 +2042,7 @@ EasyMDE.prototype.autosave = function () {
if (el != null && el != undefined && el != '') {
var d = new Date();
var dd = new Intl.DateTimeFormat([this.options.autosave.timeFormat.locale, 'en-US'], this.options.autosave.timeFormat.format).format(d);
var save = this.options.autosave.text == undefined ? 'Autosaved: ' : this.options.autosave.text;
Copy link

Choose a reason for hiding this comment

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

You change this one

Copy link
Author

Choose a reason for hiding this comment

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

But how does this affect work?

Copy link

Choose a reason for hiding this comment

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

If someone use this.options.autosave.text in this setting, this will not work anymore because you change to this.options.statusTexts.autosave. Old settings, should still work after your change.

Copy link
Author

Choose a reason for hiding this comment

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

It will work, just reset to the default value.
var statusTexts = {
lines: 'lines: ',
words: 'words: ',
autosave: 'Autosaved: ',
};

Copy link
Author

Choose a reason for hiding this comment

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

If the user adds translation of statuses, what prevents them from updating the AutoSave text? Despite the fact that it appeared recently.

Copy link

Choose a reason for hiding this comment

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

It will work, just reset to the default value.
var statusTexts = {
lines: 'lines: ',
words: 'words: ',
autosave: 'Autosaved: ',
};

But we should keep the user setting not the default setting, because this make a breakchange.

@dima-bzz
Copy link
Author

@Ionaru Hi. How exactly does the program break?

@dima-bzz
Copy link
Author

I returned to the autosave. Are there any other problems that cause this PR to fail?

@fpellet
Copy link

fpellet commented Oct 8, 2021

@Ionaru and @A-312, How to help so that it is merged?

@Ionaru
Copy link
Owner

Ionaru commented Oct 8, 2021

The autosave.text option needs to be restored, but overwritable by the new statusTexts.autosave option (currently it's the other way around). Maybe a warning can be added to autosave.text to let users know that it is deprecated.

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

Successfully merging this pull request may close these issues.

None yet

4 participants