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

ENH Use archive text when file archiving is enabled #1378

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Aug 8, 2023

Issue silverstripe/silverstripe-versioned-admin#300

AssetAdmin::getClientConfig() is an array of PHP values that are made available in JS

This pull-request is NOT updating jest tests, instead they will be updated in #1379 instead

I'm doing this because I want to test silverstripe/gha-merge-up#6 going cross major, though jest tests will definately cause merge-conflicts due to the jest tests being rewritten in 2 #1355

After merging - manually run the merge-up action on asset-admin using the default branch - https://github.com/silverstripe/silverstripe-asset-admin/actions/workflows/merge-up.yml

@@ -3,16 +3,31 @@
"AssetAdmin.BACK": "Back",
"AssetAdmin.BACK_DESCRIPTION": "Navigate up a level",
"AssetAdmin.BULK_ACTIONS_CONFIRM": "Are you sure you want to %s these files?",
"AssetAdmin.BULK_ACTIONS_ARCHIVE": "Archive",
"AssetAdmin.BULK_ACTIONS_ARCHIVE_ITEMS_CONFIRM": "You're about to archive %s file(s) which may be used in your site's content. Carefully check the file usage on the files before archiving the folder.",
Copy link
Member Author

Choose a reason for hiding this comment

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

This message is knowingly wrong where it says archiving the folder instead of archiving the file(s) - this is because this text is tested in a jest test that will be updated in #1379

"AssetAdmin.BULK_ACTIONS_DELETE_FOLDER": "These folders contain files which are currently in use, you must move or delete their contents before you can delete the folder.",
"AssetAdmin.BULK_ACTIONS_DELETE_FOLDER_CONFIRM": "Are you sure you want to delete this folder?",
"AssetAdmin.BULK_ACTIONS_DELETE_FOLDERS_CONFIRM": "Are you sure you want to delete these folders?",
"AssetAdmin.BULK_ACTIONS_DELETE_ITEMS_CONFIRM": "You're about to delete %s file(s) which may be used in your site's content. Carefully check the file usage on the files before deleting the folder.",
Copy link
Member Author

Choose a reason for hiding this comment

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

This message is knowingly wrong where it says deleting the folder instead of deleting the file(s) - this is because this text is tested in a jest test that will be updated in #1379

i18n._t(
'AssetAdmin.BULK_ACTIONS_DELETE_FAIL',
'%s folders/files were successfully deleted, but %s files were not able to be deleted.'
),
Copy link
Member Author

Choose a reason for hiding this comment

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

This key was wrong because in en.json the text is archived instead of deleted

let transKey = 'AssetAdmin.BULK_ACTIONS_DELETE_ITEMS_CONFIRM';
let transDefault = [
"You're about to delete %s file(s) which may be used in your site's content.",
'Carefully check the file usage on the files before deleting the folder.'
Copy link
Member Author

Choose a reason for hiding this comment

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

This message is knowingly wrong where it says deleting the folder instead of deleting the file(s) - this is because this text is tested in a jest test that will be updated in #1379

transKey = 'AssetAdmin.BULK_ACTIONS_ARCHIVE_ITEMS_CONFIRM';
transDefault = [
"You're about to archive %s file(s) which may be used in your site's content.",
'Carefully check the file usage on the files before archiving the folder.'
Copy link
Member Author

Choose a reason for hiding this comment

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

This message is knowingly wrong where it says archiving the folder instead of archiving the file(s) - this is because this text is tested in a jest test that will be updated in #1379

@emteknetnz emteknetnz force-pushed the pulls/1.13/archive-text branch from 17924b8 to 5374674 Compare August 8, 2023 01:25
@emteknetnz emteknetnz marked this pull request as ready for review August 8, 2023 01:25
@emteknetnz emteknetnz force-pushed the pulls/1.13/archive-text branch from 5374674 to 6d6cc96 Compare August 8, 2023 01:33
this.props.actions.toasts.success(
i18n.sprintf(
i18n._t('AssetAdmin.BULK_ACTIONS_DELETE_SUCCESS', '%s folders/files were successfully deleted.'),
Copy link
Member Author

Choose a reason for hiding this comment

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

This key was wrong because in en.json the text is archived instead of deleted

archiveFiles: PropTypes.bool,
};

BulkDeleteConfirmation.defaultProps = {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm adding default props for now for existing jest tests to pass, will be removed in #1379 and .isRequred will be added to the propType

archiveFiles: PropTypes.bool,
};

DeletionModal.defaultProps = {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm adding default props for now for existing jest tests to pass, will be removed in #1379 and .isRequred will be added to the propType

@emteknetnz emteknetnz force-pushed the pulls/1.13/archive-text branch from 6d6cc96 to 9e5df84 Compare August 8, 2023 01:58
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

A few things aren't quite right in my manual testing. Note that I checked out #1379 for this manual testing so it should have all the correct strings etc.

  1. The icon for the "archive" option is still the bin:
    Screenshot from 2023-08-08 14-52-16
    This doesn't match the icon in userhelp.

  2. The icon at the bottom of the admin for archiving images/files when you select multiple (using the check boxes) is still the bin
    Screenshot from 2023-08-08 14-57-57

@emteknetnz emteknetnz force-pushed the pulls/1.13/archive-text branch from 9e5df84 to fa0fef2 Compare August 8, 2023 03:25
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks good, works well locally.

@GuySartorelli GuySartorelli merged commit 3e5afba into silverstripe:1.13 Aug 8, 2023
@GuySartorelli GuySartorelli deleted the pulls/1.13/archive-text branch August 8, 2023 04:08
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.

2 participants