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

percentage-fontsize plugin implementation #688

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

abetis
Copy link
Contributor

@abetis abetis commented Apr 11, 2018

The plugin allows specifying font size as a percent value as used in phpBB system.

specifying font size as a percent value as used in phpBB system.
@abetis abetis force-pushed the percentage-fontsize-plugin branch from 344f1d2 to 28e44d7 Compare April 11, 2018 12:48
@abetis abetis changed the title percentage-fontsize implementation percentage-fontsize plugin implementation Apr 11, 2018
Copy link
Collaborator

@brunoais brunoais left a comment

Choose a reason for hiding this comment

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

For now, just those. I'll think about the (non-obvious) others when able.

* @memberOf SCEditor.prototype
*/
base.wysiwygGetTags = function (tagName) {
return wysiwygDocument.getElementsByTagName(tagName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This returns a live ordered collection of Elements by tag name.
Either be explicit about it in its documentation or change the code into something most programmers would expect from it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check other methods, maybe it will be easier to just inject the HTML tags instead of using execCommand

* @type {Object}
* @private
*/
var sizes = [50, 75, 100, 150, 200];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this behave like a constant? If so, turn the variable name into CAPS
Personally, I'd have a default value but allow setting options to change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. updating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a documentation for plugin configuration fomat?
Couldn't find any documentation for format.js configuration for example...

sceditor.command.set('size', {
exec: commandHandler,
txtExec: commandHandler,
tooltip: 'Font Size'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use _ function to make it translatable or allow it to be translable somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please explain what do you mean here.
_ for what function?
What does "translatable" means?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a function named _. See: https://www.sceditor.com/api/sceditor/underscore/
"translatable" <=> To be able to have a different text on the node based on the current language

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I don't understand what function do you mean that have to be with underscore or translatable.
I'm basing my code the same way as is appears in the format plugin that Sam wrote and as defaultCommands.js code looks like...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, OK. The code sends tooltips through the translation function when displaying:
https://github.com/samclarke/SCEditor/blob/master/src/lib/SCEditor.js#L712
I had forgotten about that.

var size;

if (!fontSize) {
fontSize = element.style['font-size'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

element.style['font-size'] -> element.style.fontSize

@brunoais brunoais requested a review from samclarke April 11, 2018 13:15
Change the method to inject the HTML.
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.

None yet

2 participants