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 option to specify icons set #55

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

Conversation

roipoussiere
Copy link

This PR adds a new option iconsSet, to let the developper specify which icons set to use.

Currently, font-awesome (fa) or material (material) are supported, but others can be easily added.

Usage:

var easymde = new EasyMDE({
    element: $('#articleContent')[0],
    iconsSet: 'material'
});

The following will produce this toolbar:

screenshot_20190211_163631

Note that font-awesome is kept by default, in order to don't introduce changes to current version.

So this:

var easymde = new EasyMDE({
    element: $('#articleContent')[0],
    iconsSet: 'fa'
});

or this

var easymde = new EasyMDE({
    element: $('#articleContent')[0]
});

produces this, as usual:

screenshot_20190211_163708

@roipoussiere roipoussiere mentioned this pull request Feb 11, 2019
@roipoussiere
Copy link
Author

Please don't merge, this introduces some breaks on icons I didn't check.

@roipoussiere
Copy link
Author

I think it's okay. The problems was mainly with header icons, I did not check H+, H-, H1, H2 and H3.

It was buggy both for fa-4, fa-5 and Material.

Note that this was not specially a regression bug: these icons are also buggy on current master and dev branches.

So the rendering of all icons is now:

for Font-awesome 4 :

screenshot_20190212_182352

for Font-awesome 5 :

screenshot_20190212_182518

for Material icons :

screenshot_20190212_185343

Ready to merge for me.

Copy link
Owner

@Ionaru Ionaru left a comment

Choose a reason for hiding this comment

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

PR is based on master, this should be development.

EDIT: actually, ignore that. I am dropping the development branch.

README.md Outdated Show resolved Hide resolved
@roipoussiere
Copy link
Author

PR is based on master, this should be development.

EDIT: actually, ignore that. I am dropping the development branch.

Could you merge development into master then? In this way I can fix conflicts in readme on this PR.

@Ionaru
Copy link
Owner

Ionaru commented Feb 20, 2019

Could you merge development into master then? In this way I can fix conflicts in readme on this PR.

Done!

@roipoussiere
Copy link
Author

roipoussiere commented Feb 20, 2019

Done!

Thank you! With ed96d18 this branch is up to date with master and can be merged safely. :)

You should be able to use rebase and merge option to avoid creating an other merge commit. ;)

@@ -1145,58 +1148,114 @@ var toolbarBuiltInButtons = {
'bold': {
name: 'bold',
action: toggleBold,
className: 'fa fa-bold',
className: {
Copy link
Owner

Choose a reason for hiding this comment

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

Changing the className to this new object would introduce a breaking change for people that use the custom toolbar.
I'd rather not break compatibility at this time to make the transition from SimpleMDE -> EasyMDE as painless as possbile.

We could introduce an icon field in the toolbar button options, that looks like this:

// FontAwesome icon
'bold': {
    name: 'bold',
    action: toggleBold,
    icon: {
        fa: '<i class="fa fa-bold"></i>',
        material: '<i class="material-icons">format_bold</i>'
    },
    ...
},

This way we can simply pick the correct icon depending on the chosen (or default) iconsSet.
One thing to keep in mind though is to still allow overwriting the FontAwesome icon from className (which won't work with material icons).

We might be making the API unnecessarily complicated, perhaps we should delay the feature until a full rewrite is possible.

Copy link
Author

@roipoussiere roipoussiere Feb 27, 2019

Choose a reason for hiding this comment

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

What about this?

'bold': {
    name: 'bold',
    action: toggleBold,
    icon: {
        'fa': {'className': 'fa fa-italic'},
        'material': {'className': 'material-icons', 'textContent': 'format_italic'},
    }
    ...
},

And something like this (not tested) in createIcon():

var icon = document.createElement('i');
if(options.icon && options.icon[iconsSet || 'fa']) {
    // if options.icon is provided among with a correct icon set,
    // look for className and textContent attributes and defaults to fa values
    icon.className = options.icon[iconsSet || 'fa']['className'] || options.className;
    icon.textContent = options.icon[iconsSet || 'fa']['textContent'] || '';
} else {
    // use options.className if options.icon is not provided
    icon.className = options.className
}

@nerg4l
Copy link

nerg4l commented Dec 10, 2020

Is there anyway I can help adding this feature? Material Icons is already added to one of my applications. With this feature I could avoid including an extra icon set and make the icons consistent overall.

@Chaostheorie
Copy link

It would be nice to add support for bootstrap-icons on that note too. They're licensed under MIT License and could include with their SVGs.

@vanillajonathan
Copy link
Contributor

Another approach would be a dictionary/map with key-values to define the class names to use.
#448

@roipoussiere
Copy link
Author

Hi everyone, I don't use easyMde currently so I'm probably not going to continue to work on this PR. If anyone interested, please fork my fork and create a new PR.

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

Successfully merging this pull request may close these issues.

5 participants