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

[staging] Fix tinymce ctrl-s #27519

Merged
merged 3 commits into from Jan 21, 2020
Merged

[staging] Fix tinymce ctrl-s #27519

merged 3 commits into from Jan 21, 2020

Conversation

@brianteeman
Copy link
Contributor

brianteeman commented Jan 14, 2020

PR for #16550

The tinymce save plugin doesnt work in joomla and instead of just doing nothing it discards edited article instead of saving it

This PR stops loading the save plugin so now if you press ctr-s you will just get any dialog your browser has set. Most importantly it does not result in you losing work

PR for #16550

The tinymce save plugin doesnt work in joomla and instead of just doing nothing it  discards edited article instead of saving it

This PR stops loading the save plugin so now if you press ctr-s you will just get any dialog your browser has set. Most importantly it does not result in you losing work
@brianteeman

This comment has been minimized.

Copy link
Contributor Author

brianteeman commented Jan 14, 2020

@wilsonge I can port this super simple change to j4 if you dont want to have to deal with a merge

@jwaisner

This comment has been minimized.

Copy link

jwaisner commented Jan 14, 2020

I have tested this item successfully on 5dd7cf7


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27519.

1 similar comment
@viocassel

This comment has been minimized.

Copy link
Contributor

viocassel commented Jan 14, 2020

I have tested this item successfully on 5dd7cf7


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27519.

@Quy Quy removed the PR-staging label Jan 14, 2020
@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Jan 14, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27519.

@joomla-cms-bot joomla-cms-bot added the RTC label Jan 14, 2020
@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 14, 2020

@brianteeman at one point in the very first version of atum @dgrammatiko had the control + s actually saving the article. I'd rather get that functionality restored again than doing a direct port of removing the save plugin for j4 (obviously this is fine for j3)

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Jan 14, 2020

@wilsonge there is a PR for that #24152 (although the js part should be part of the toolbar custom element)

@brianteeman

This comment has been minimized.

Copy link
Contributor Author

brianteeman commented Jan 14, 2020

even that pr requires this patch to work

I wanted to update that PR to use https://github.com/github/hotkey which is much more powerful but struggled with the "import"

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Jan 15, 2020

I was reluctant on the es6 approach back in the days because what we have is a bastard between modern and legacy js. That project (the es6 transition) was never actually completed. In sort you cannot use import without a bundled like Webpack or rollup...

@brianteeman

This comment has been minimized.

Copy link
Contributor Author

brianteeman commented Jan 15, 2020

:( thats a shame because #24152 is restricted to using regular modifier keys and they always conflict

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Jan 15, 2020

@brianteeman nope, that's fixable, eg:

	if (e.altKey && e.which == 83) {
                e.preventDefault();
 		var toolbar = document.getElementById("toolbar-apply");
 		toolbar.getElementsByClassName("button-apply")[0].click(); 
 	};

and change the event to fire onkeydown not onkeyup

PS. FWIW that code should be part of the toolbar CE js not another random js file

@brianteeman

This comment has been minimized.

Copy link
Contributor Author

brianteeman commented Jan 15, 2020

I dont want to do e.preventDefault(); as I consider it a bad idea to replace existing hotkeys - they might not be coming just from the browser (hint screenreaders) and thats why github, gmail etc all use a different modifier. (I can do it easily with mousetrap but you wouldnt let me)
and no it should not be part of the toolbar - the toolbar buttons are only a subset of where it could be used and we shouldnt limit ourselves

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Jan 15, 2020

the toolbar buttons are only a subset of where it could be used and we shouldnt limit ourselves

Let me disagree here. You try to cover cases that nobody actually asked for and probably will be used by a minority of the users. You are making this thing one more plugin. Did anyone counted how many plugins J4 is introducing?
Just add the basic shortcuts that people expect to work there (ctr+S was the obvious one all the others are debatable).
Again adding features just to end up saying "hey Joomla provides also that" although it would be extremely hard to manage it and not to mention the customisation part. Why? Just why?
Simplicity is the ultimate design...

@brianteeman

This comment has been minimized.

Copy link
Contributor Author

brianteeman commented Jan 15, 2020

rofl says the man who introduced hundreds of javascript files

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Jan 15, 2020

rofl says the man who introduced hundreds of javascript files

You do understand that you comment is irrelevant. Did I actually brought to the project a deprecated/outdated script like the one from eBay or the mousetrap that you mentioned above? No. So please...

@C-Lodder

This comment has been minimized.

Copy link
Member

C-Lodder commented Jan 15, 2020

Is it just me or does this feature keep getting broken in J4?

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 15, 2020

It only got broken once. and then never fixed. so not keep getting broken no :P

@brianteeman

This comment has been minimized.

Copy link
Contributor Author

brianteeman commented Jan 15, 2020

It never worked in joomla 3

@brianteeman

This comment has been minimized.

Copy link
Contributor Author

brianteeman commented Jan 15, 2020

The original report was in June 2017

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Jan 15, 2020

That plugin rightfully needs to be removed:
Screenshot 2020-01-15 at 16 04 42

This PR has nothing to do with the ctrl+S combo @wilsonge and @C-Lodder talking about. That is another conversation, what the project should offer by default and why. Also the implementation needs to be tightly connected to the toolbar as the events actually trigger those buttons...

My 2c

@brianteeman

This comment has been minimized.

Copy link
Contributor Author

brianteeman commented Jan 15, 2020

Just merge this and get on with it

@Quy Quy added the PR-staging label Jan 15, 2020
@brianteeman

This comment has been minimized.

Copy link
Contributor Author

brianteeman commented Jan 21, 2020

Having discussed the use of custom keys and/or modifying keys used by the browser with Bruce regarding accessibility it was deemed to be a bad idea to change the expected behaviour of a browser

@wilsonge wilsonge merged commit e9613c9 into joomla:staging Jan 21, 2020
2 of 4 checks passed
2 of 4 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/drone/pr Build is running
Details
Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 21, 2020

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC label Jan 21, 2020
@wilsonge wilsonge added this to the Joomla! 3.9.15 milestone Jan 21, 2020
@brianteeman brianteeman deleted the brianteeman:save branch Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.