Added option to use icons (like glyphicons) and LESSified styles, added min.css #25

falxcerebri · 2016-09-19T10:54:44Z

If you're using gulp or other task automation and you're including stylesheets inside a main stylesheet you'll probably find this useful.

To use icons add (say you're using glyphicons): `ico: ["glyphicons", "glyphicon-edit"]`

s-yadav · 2016-09-22T17:34:40Z

contextMenu.css

@@ -64,6 +64,6 @@
  height: 20px;
  left: 0px;
  top: 3px;
-  position: absolute;


Why position is removed ?

That's just a typo. I'm working on two repositories and got thing messed up. It shouldn't be removed.

s-yadav · 2016-09-22T17:35:49Z

contextMenu.js

@@ -103,6 +103,7 @@
                        var name = selector[i].name,
                            disable = selector[i].disable,
                            fun = selector[i].fun,
+							ico = selector[i].ico,


Can you maintain the indentation?

s-yadav · 2016-09-22T17:40:39Z

contextMenu.js

@@ -135,6 +136,26 @@
                            }
                        }

+						//update ico
+                        if (ico) {


While create its inside else if, but here its new if. This changes which one will be used among img/ico on create and update. Can you fix that.
And if ico was defined on create and later on update is defined than it will not remove the span element.

s-yadav · 2016-09-22T17:42:17Z

contextMenu.js

+                        if (ico) {
+                            var imgIcon = elm.find('.iw-mIcon');
+                            if (imgIcon.length) {
+								if(imgIcon[0].tagName.toLowerCase() === "span"){


As its a jquery dependent library. You can use .is() method. ie imgIcon.is('span')

s-yadav · 2016-09-22T17:45:46Z

contextMenu.js

+                            var imgIcon = elm.find('.iw-mIcon');
+                            if (imgIcon.length) {
+								if(imgIcon[0].tagName.toLowerCase() === "span"){
+									var classList = ["iw-mIcon"];


You can just write imgIcon.attr('class', 'iw-mIcon '+ico). And always accept ico as string. If multiple class is required it can be added as space seperated ('icon-add align-left')

I didn't thing of adding multiple classes as string with spaces. That simplifies stuff a lot. 👍

s-yadav · 2016-09-22T17:50:36Z

contextMenu.js

+                                elm.prepend('<span align="absmiddle" class="iw-mIcon '+ico.toString().replace(",", " ")+'" />');
+                            }
+                        }
+


I feel it will be better just remove iw-mIcon and prepend it again based on ico/img. Something like this.

if(imgIcon){ imgIcon.remove(); } if (img) { elm.prepend('<img src="' + img + '" align="absmiddle" class="iw-mIcon" />'); } else if (ico) { elm.prepend('<span align="absmiddle" class="' + ("iw-mIcon "+ico+'" />'); }

s-yadav · 2016-09-22T17:51:03Z

contextMenu.js

@@ -681,15 +702,17 @@
                        fun = selObj.fun || function() {},
                        subMenu = selObj.subMenu,
                        img = selObj.img || '',
+						ico = selObj.ico || '',


Indentation.
Also I feel ico can be iconClass or just icon.

s-yadav · 2016-09-22T17:51:34Z

contextMenu.js

-                    }
-
+                    } else if (ico) {
+						list.prepend('<span align="absmiddle" class="' + ("iw-mIcon "+ico.toString().replace(","," "))+'" />');


Accept ico only as string

falxcerebri · 2016-09-22T18:47:31Z

Cool! Thanks for suggestions. I'll fix them ASAP and prepare commits.

…dating logic

falxcerebri · 2016-09-22T22:18:19Z

Let me know what you think now. I'm excited to get some more feedback.

s-yadav

Everything looks great.
Just check if we can optimize .less file.

Or you can send me a PR leaving the less file for now. I will merge the icon feature. Less file we can merge in next PR.

s-yadav · 2016-09-23T10:51:13Z

contextMenu.less

+ *Lessified using http://css2less.co/
+ */
+// Color variables (appears count calculates by raw css)
+@color0: #333333; // Appears 2 times


All colors are variation of black and white.
Can we just have two base colors and use lighten or darken method of less to provide variation? This will help a lot on customizing the theme.

It's hard to reduce more of them, you still need white for overlay, and I'd leave background and highlight colors as parameters

falxcerebri · 2016-09-23T14:05:20Z

Let me know what you think abut LESS now.

s-yadav · 2016-09-24T13:46:02Z

contextMenu.js

@@ -125,22 +126,21 @@
                        //update class name
                        className != undefined && elm.attr('class', className);

-                        //update image
+                        var imgIcon = elm.find('.iw-mIcon');
+                        if(imgIcon.length) imgIcon[0].remove();


This should be imgIcon.remove();
imgIcon[0] will give DOM object and there is no remove method of DOM object (Thats a jquery method)

falxcerebri · 2016-09-26T08:39:43Z

👍

falxcerebri added 2 commits September 19, 2016 12:53

Lessified css file, for folks who use LESS

59eabf2

If you're using gulp or other task automation and you're including stylesheets inside a main stylesheet you'll probably find this useful.

Added option to use icon files of your choice. Minified css as well.

b74a4f7

To use icons add (say you're using glyphicons): `ico: ["glyphicons", "glyphicon-edit"]`

falxcerebri changed the title ~~Lessified css file, for folks who use LESS~~ Added option to use icons (like glyphicons) and LESSified styles, added min.css Sep 22, 2016

falxcerebri mentioned this pull request Sep 22, 2016

Add option to use icons #26

Open

There was a problem with the styles from LESS. Fixed.

463bfe2

s-yadav reviewed Sep 22, 2016

View reviewed changes

Fixed indentation, renamed ico to icon, improved icon creation and up…

02f8215

…dating logic

s-yadav reviewed Sep 23, 2016

View reviewed changes

Improved LESS variables.

7679475

It's hard to reduce more of them, you still need white for overlay, and I'd leave background and highlight colors as parameters

s-yadav reviewed Sep 24, 2016

View reviewed changes

Fixed as requested

89b6149

s-yadav approved these changes Sep 26, 2016

View reviewed changes

s-yadav merged commit b022af3 into s-yadav:master Sep 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option to use icons (like glyphicons) and LESSified styles, added min.css #25

Added option to use icons (like glyphicons) and LESSified styles, added min.css #25

falxcerebri commented Sep 19, 2016

s-yadav Sep 22, 2016

falxcerebri Sep 22, 2016

s-yadav Sep 22, 2016

s-yadav Sep 22, 2016

s-yadav Sep 22, 2016

s-yadav Sep 22, 2016

falxcerebri Sep 22, 2016

s-yadav Sep 22, 2016

s-yadav Sep 22, 2016 •

edited

Loading

s-yadav Sep 22, 2016

falxcerebri commented Sep 22, 2016

falxcerebri commented Sep 22, 2016

s-yadav left a comment

s-yadav Sep 23, 2016

falxcerebri commented Sep 23, 2016

s-yadav Sep 24, 2016

falxcerebri commented Sep 26, 2016

Added option to use icons (like glyphicons) and LESSified styles, added min.css #25

Added option to use icons (like glyphicons) and LESSified styles, added min.css #25

Conversation

falxcerebri commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-yadav Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falxcerebri commented Sep 22, 2016

falxcerebri commented Sep 22, 2016

s-yadav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falxcerebri commented Sep 23, 2016

Choose a reason for hiding this comment

falxcerebri commented Sep 26, 2016

s-yadav Sep 22, 2016 •

edited

Loading