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

Better description for Shrink Database button #4508

Draft
wants to merge 4 commits into
base: 1.23.X
Choose a base branch
from

Conversation

Saibamen
Copy link
Contributor

@Saibamen Saibamen commented Feb 20, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

As discussed in #2470 (comment)

Previous description suggested that you need to recreate your database from scratch.

Type of change

Please delete any options that are not relevant.

  • Other

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

image

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Changing the meaning of a translation is not great, as this leaves the translators unaware of this change.

A better approach would be to introduce a translation string and remove the current one.
This also allows using the i18n-t-component to display VACUUM/AUTO_VACUUM as <code>VACUUM</code>/<code>AUTO_VACUUM</code>
Here is the relevant line:

<div class="form-text mt-2 mb-4 ms-2">{{ $t("shrinkDatabaseDescription") }}</div>

@Saibamen
Copy link
Contributor Author

Changing the meaning of a translation is not great, as this leaves the translators unaware of this change.

I translated via Weblate and I bet that I translated few already translated i18n keys with changed English source.

But I like to have HTML's <code>. I will change that today. Thanks

@CommanderStorm CommanderStorm changed the title Update en.json: better description for Shrink Database button Better description for Shrink Database button Feb 21, 2024
@CommanderStorm CommanderStorm marked this pull request as draft February 21, 2024 20:11
@Saibamen

This comment was marked as resolved.

@Saibamen Saibamen marked this pull request as ready for review February 22, 2024 09:58
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I have made a few changes.
Given that you touched the other translation files, this PR can only be merged directly after merging #4436 and #4394 to avoid merge conflicts being created in Weblate.

src/components/settings/MonitorHistory.vue Outdated Show resolved Hide resolved
src/components/settings/MonitorHistory.vue Outdated Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
src/components/settings/MonitorHistory.vue Outdated Show resolved Hide resolved
@CommanderStorm
Copy link
Collaborator

There may be other merge conflicts that come up. v2.0 does support mariadb next to sqlite, but said db still needs to be integrated here as well

@Saibamen
Copy link
Contributor Author

Saibamen commented Feb 22, 2024

This PR is for 1.23.x only

@CommanderStorm CommanderStorm added this to the 1.23.12 milestone Feb 28, 2024
@louislam
Copy link
Owner

louislam commented Mar 3, 2024

Since merging conflicts on Weblate is painful, as the git repo is read only. And removing the key is likely causing this problem.

I think we need to find another way to "deprecated" this key instead of removing it.

@louislam louislam added the question Further information is requested label Mar 3, 2024
@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Mar 3, 2024

I think this should not cause a merge-conflict if merged directly after #4436
Am I missing some context?

What might indeed be problematic is that users can't supply translations for v1.23.X, only for v2.X
=> this change will never get translated.

@louislam
Copy link
Owner

louislam commented Mar 3, 2024

I think this should not cause a merge-conflict if merged directly after #4436 Am I missing some context?

Since it is for the 1.23.X branch, it will happen when I merge the 1.23.x branch to master, I believe.

Of course I can merge the 1.23.X branch to master after #4436, but it is kind of "soft-locked".

What might indeed be problematic is that users can't supply translations for v1.23.X, only for v2.X => this change will never get translated.

You are right. And 1.23.X is indeed not translatable in the current stage, as Weblate don't support for multiple branches. (Or I don't know how to set up)

For simplicity, I would suggest this pr should be merged into the master branch directly and also be merged after #4436.

@CommanderStorm CommanderStorm marked this pull request as draft March 3, 2024 15:06
@CommanderStorm CommanderStorm modified the milestones: 1.23.12, 2.0.0 Mar 3, 2024
@Saibamen
Copy link
Contributor Author

Saibamen commented Mar 3, 2024

@Saibamen

This comment was marked as resolved.

@CommanderStorm CommanderStorm added the needs:work this PR needs work label May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:work this PR needs work question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants