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

Add reloading of lexicon entries for any namespace #16333

Closed
wants to merge 2 commits into from
Closed

Add reloading of lexicon entries for any namespace #16333

wants to merge 2 commits into from

Conversation

Ruslan-Aleev
Copy link
Collaborator

@Ruslan-Aleev Ruslan-Aleev commented Dec 21, 2022

What does it do?

Add reloading of lexicon entries for any namespace:

  • Move the button up, it's was almost invisible.
  • Added a pop-up window with confirmation, where the namespace, topic and language are indicated.
  • The name of the strings is now returned to the console, and not just their amount.

lexicons_reload

p.s. In a good way, this PR should be improved and merged after this (#15942), although the current version is also working.

Related issue(s)/PR(s)

#14442

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Dec 21, 2022
@Ruslan-Aleev Ruslan-Aleev added area-lexicons Issues related to translations and lexicons implementation. pr/review-needed Pull request requires review and testing. labels Dec 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2022

Codecov Report

Base: 16.91% // Head: 17.84% // Increases project coverage by +0.92% 🎉

Coverage data is based on head (5333c5a) compared to base (680df53).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##                3.x   #16333      +/-   ##
============================================
+ Coverage     16.91%   17.84%   +0.92%     
+ Complexity    10447    10440       -7     
============================================
  Files           561      561              
  Lines         38028    39042    +1014     
============================================
+ Hits           6433     6967     +534     
- Misses        31595    32075     +480     
Impacted Files Coverage Δ
...on/Processors/Workspace/Lexicon/ReloadFromBase.php 0.00% <0.00%> (ø)
core/src/Revolution/modDocument.php 50.00% <0.00%> (-12.50%) ⬇️
core/src/Revolution/Processors/Element/Get.php 50.00% <0.00%> (-5.00%) ⬇️
core/src/Revolution/Processors/Context/GetList.php 75.00% <0.00%> (-5.00%) ⬇️
core/src/Revolution/Processors/Element/GetList.php 78.57% <0.00%> (-4.77%) ⬇️
core/src/Revolution/Processors/Element/Update.php 63.04% <0.00%> (-4.40%) ⬇️
...olution/Processors/Element/PropertySet/GetList.php 48.78% <0.00%> (-4.00%) ⬇️
...ssors/Security/User/GetRecentlyEditedResources.php 27.14% <0.00%> (-3.51%) ⬇️
...e/src/Revolution/Processors/System/ConfigCheck.php 76.22% <0.00%> (-2.67%) ⬇️
core/src/Revolution/modAccessCategory.php 53.84% <0.00%> (-2.41%) ⬇️
... and 108 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@smg6511 smg6511 left a comment

Choose a reason for hiding this comment

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

First off, nice job extending this to reversion of Lexicons beyond the core ones! Re my suggestions — they are mostly of the fine-tuning variety and this PR works as intended without them, so I intend to approve once you get a look. I am hoping you'll agree with most of what I proposed though ;-)

For the future: I do think it's a bit of a weird UI move to use the MODX console here. This has nothing to do with @Ruslan-Aleev's work (it was already that way). I think that should be reserved only for installation procedures and cache-clearing. At worst a regular Window showing success/fail feedback makes more sense; at best IMO we move toward a UI model where the user doesn't have to click or otherwise be interrupted by this feedback (such as a "Toast"-like message that is less obtrusive and disappears on its own). That's a job for another day.

Better-yet, for this area and others like it, a more elegant flow would report how many and which items are about to be changed (and from "x" to "y") in the confirmation window and have the final notification window (or Toast) simply say that the action succeeded (or failed).

Comment on lines +29 to +31
$_lang['lexicon_revert_confirm'] = 'Are you sure you want to revert the "[[+namespace]]" namespace > "[[+topic]]" topic > "[[+language]]" lexicon to its default strings?';
$_lang['lexicon_revert_success'] = 'Reverted [[+total]] total string(s): [[+names]]';
$_lang['lexicon_revert_error'] = 'No lexicon entries found to revert.';
Copy link
Collaborator

Choose a reason for hiding this comment

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

The language used still is a bit clunky IMO and the term strings is not going to be clear to most users unfamiliar with coding. Consider the following:

Suggested change
$_lang['lexicon_revert_confirm'] = 'Are you sure you want to revert the "[[+namespace]]" namespace > "[[+topic]]" topic > "[[+language]]" lexicon to its default strings?';
$_lang['lexicon_revert_success'] = 'Reverted [[+total]] total string(s): [[+names]]';
$_lang['lexicon_revert_error'] = 'No lexicon entries found to revert.';
$_lang['lexicon_revert_confirm'] = 'This action will permanently replace all overridden Lexicon entries in the selected namespace/topic/language with their original text. User-created entries will not be affected. Continue?';
$_lang['lexicon_revert_multiple_success'] = 'Successfully reverted [[+total]] overridden entries: [[+names]]';
$_lang['lexicon_revert_success'] = 'Successfully reverted one overridden entry: [[+names]]';
$_lang['lexicon_revert_error'] = 'No Lexicon entries found to revert.';

Note that the only change to the final entry is capitalization of “Lexicon.”

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. I'm still in favor of displaying both "[[+namespace]]", and "[[+topic]]", and "[[+language]]" in the message, because pop-up window may cover the selection in the grid and it will not be clear what is selected.
  2. @rthrash @smg6511 Can you decide on the final spelling? So that the final version is as clear or we can leave the current one?

manager/assets/modext/workspace/lexicon/lexicon.grid.js Outdated Show resolved Hide resolved
manager/assets/modext/workspace/lexicon/lexicon.grid.js Outdated Show resolved Hide resolved
@Ruslan-Aleev Ruslan-Aleev requested a review from Mark-H January 26, 2023 19:45
@Ruslan-Aleev Ruslan-Aleev deleted the 3.x-feature-14442 branch January 26, 2023 20:54
opengeek added a commit that referenced this pull request Mar 25, 2024
### What does it do?
Add reloading of lexicon entries for any namespace:

- Move the button up, it's was almost invisible.
- Added a pop-up window with confirmation, where the namespace, topic
and language are indicated.
- The name of the strings is now returned to the console, and not just
their amount.


![lexicons_reload](https://user-images.githubusercontent.com/12523676/208910173-49426e6e-a70a-4c55-9ced-4e1a0afdcb5a.gif)

p.s. In a good way, this PR should be improved and merged after this
(#15942), although the current
version is also working.

### Related issue(s)/PR(s)
#16333
#14442

---------

Co-authored-by: Jason Coward <jason@opengeek.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-lexicons Issues related to translations and lexicons implementation. cla-signed CLA confirmed for contributors to this PR. pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants