Skip to content

Conversation

@anuragteapot
Copy link
Contributor

Pull Request for Issue #23755

Summary of Changes

PR after a long time 😛

Media manager repo is not active these days thats why i am creating PR here.

  1. Now delete is using alert.
  2. Error if item not seleted.
  3. Change in logic : Unselect all selected item except that item, if single item is going to delete.

screenshot from 2019-02-03 16-57-37
screenshot from 2019-02-03 16-57-40

Testing Instructions

Try to use delete you should see alert.

Expected result

you should see alert.

Actual result

Alert not showing for toolbar delete.

Documentation Changes Required

NO

@dgrammatiko

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Feb 3, 2019
@dgrammatiko
Copy link
Contributor

dgrammatiko commented Feb 3, 2019

Nice!

Can I ask for one more PR: update to the latest version of the deps (vue, etc) and also devDependencies (Webpack, etc)?

Anu1601CS added 2 commits February 3, 2019 17:56
@anuragteapot
Copy link
Contributor Author

Nice!

Can I ask for one more PR: update to the latest version of the deps (vue, etc) and also
devDependencies (Webpack, etc)?

Thanks
All done.

@infograf768
Copy link
Member

Works fine. 👍

As I guess we can't use Plural form in our js, I suggest to change the string
from
COM_MEDIA_CONFIRM_DELETE_MODEL_DESC="Are you sure you want to delete this item?"
to rather use JGLOBAL_CONFIRM_DELETE as the string is quite neuter.

JGLOBAL_CONFIRM_DELETE="Are you sure you want to delete? Confirming will permanently delete the selected item(s)!"

which is the global alert in core.

@anuragteapot
Copy link
Contributor Author

@infograf768

As I guess we can't use Plural form in our js, I suggest to change the string
from
COM_MEDIA_CONFIRM_DELETE_MODEL_DESC="Are you sure you want to delete this item?"
to rather use JGLOBAL_CONFIRM_DELETE as the string is quite neuter.

JGLOBAL_CONFIRM_DELETE="Are you sure you want to delete? Confirming will permanently delete the selected item(s)!"
which is the global alert in core.

I think for this another PR is fine :)

@rdeutz
Copy link
Contributor

rdeutz commented Feb 4, 2019

npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with npm install before continuing.

Something didn't work while changing the package.json

@infograf768
Copy link
Member

weird. It worked yesterday. I get that error today

@anuragteapot
Copy link
Contributor Author

Now I think it should be fine.

@dgrammatiko I have updated root package.json. Should I leave the changes or revert this one?

@anuragteapot
Copy link
Contributor Author

@wilsonge Done

wilsonge added a commit to joomla/test-system that referenced this pull request Feb 25, 2019
@wilsonge
Copy link
Contributor

I think joomla/test-system#53 should solve the tests here

@anuragteapot
Copy link
Contributor Author

I think joomla/test-system#53 should solve the tests here

@wilsonge

I think yes. Instead of direct delete, we need to click modal confirm button.

wilsonge added a commit to joomla/test-system that referenced this pull request Feb 25, 2019
* Fix system tests when adding modal

joomla/joomla-cms#23756

* Ensure the modal loads
@wilsonge
Copy link
Contributor

@rdeutz can i have some help here please. I merged the system tests fix - but the composer update doesn't seem to be making a difference here. Is that related to cache'ing or am i doing something stupid?

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Feb 25, 2019

@wilsonge use [NO CACHE] in the title, eg
[4.0][NO CACHE] Fix Media Manager delete alert not used.

@wilsonge wilsonge changed the title [4.0] Fix Media Manager delete alert not used. [4.0] [NO CACHE] Fix Media Manager delete alert not used. Feb 25, 2019
@wilsonge
Copy link
Contributor

Done. It did skip the restore-cache step - but still not getting the updated test

@wilsonge
Copy link
Contributor

OK Packagist hasn't actually updated with the hash of the new commit :( i don't know if there's even a good way to trigger that https://packagist.org/packages/joomla/test-system#dev-4.0-dev

@dgrammatiko
Copy link
Contributor

Well, that's the docs for the caching but I guess the problem is in the composer.
screenshot 2019-02-25 at 19 13 02

Can you change

- composer update joomla/test-system --no-progress --no-suggest
so it fetches the head similar to the npm approach?

@wilsonge
Copy link
Contributor

@rdeutz there's no webhook to packagist configured for the system test repo - i don't own the packagist repo so you need to fix that i'm afraid

@rdeutz
Copy link
Contributor

rdeutz commented Feb 26, 2019

[NO CACHE] must be part of the commit message

webhooks are all fine

@wilsonge
Copy link
Contributor

@SniperSister @zero-24 can you have a look at RIPS here please

@wilsonge wilsonge merged commit 5ffbf8e into joomla:4.0-dev Feb 27, 2019
@wilsonge
Copy link
Contributor

Thanks!

@infograf768
Copy link
Member

Please test #24258

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants