Remove octicons altogether from gollum-lib.

[bartkamphorst]

This is an alternative to #440. This PR removes the octicon dependency entirely from gollum-lib. Instead of generating an icon svg in gollum-lib, the relevant macros instead output divs with up to three data-* attributes (data-gollum-icon, and optionally data-gollum-icon-height and data-gollum-icon-width). The idea would be that gollum would subsequently use these data-attributes to look up the right icon (for the time being, octicon) and inject it into the DOM. This way, the octicon gem is only a dependency for gollum, and no longer for gollum-lib.

This PR also renames the Octicon Macro to Icon.

[bartkamphorst]

In response to @benjaminwil in #440 about having to update two gems if we ever decide to change icon sets (or if octicons change their icon names), let me add the following. Right now, I've opted to use the same mechanism of setting data-gollum-icon also for icon defaults, such as data-gollum-icon="zap" in the Macro error flash div, or data-gollum-icon="alert" in the Warn flash div. This means that gollum-lib effectively still references three octicon names: ['zap', 'info', 'alert'].

If we want to avoid this altogether, we could choose to leave those hardcoded names out, and only supply a data-gollum-icon field for user input (e.g., data-gollum-icon="bell" if and only if <<Note('message', 'bell')>>).

[benjaminwil]

If we want to avoid this altogether, we could choose to leave those hardcoded names out, and only supply a data-gollum-icon field for user input (e.g., data-gollum-icon="bell" if and only if <<Note('message', 'bell')>>).

I didn't realize that the <<Note>> macro takes an icon argument already. (The <<Warning>> macro doesn't.)

If it were up to me alone, I would change the macro method signatures to not take icons as arguments so we can further separate the presentational code in gollum from the document-rendering code of gollum-lib.

Using only CSS class names, we can indicate to gollum that a note, warning, or alert, should be presented a certain way:

<!-- `gollum-lib` note macro output-->
<div class="flash flash-note macro-note">
  ...
</div>

/* `gollum`-provided CSS for the note macro. */
.flash.macro-note::before {
  content: url('path-to-icon.svg');
  width: 64px;
  /* etc... */
}

End users could still provide a custom icon using custom CSS without having to make any changes to pages using the macro:

/* Custom CSS file. */
.flash.macro-note {
  background-color: var(--custom-background-color);
}

.flash.macro-note::before {
   content: url('path-to-custom-icon.svg');
}

Alternatively, it would be nice if the macros could be moved to the gollum codebase somehow. Then we can have as much presentational logic in them as we want at a much less expensive maintenance cost.

---

All that said, I still like this change as-is and think it's an improvement for gollum-lib to not explicitly depend on Octicons.

[bartkamphorst]

What about if we style / add icons in gollum's CSS for macro error, warning and note, but we add a separate "Flash" macro that people can style themselves? Since macros do allow for HTML input, they could then even do something like <<Flash("<div data-gollum-icon='bell'></div>Important message here")>>.

[benjaminwil]

I really like the idea of a flash macro that can have customizable styles. On the other hand, I could see an argument for providing a customizable macro templates every place we document custom Gollum macros, since we can't anticipate exactly how custom someone might want their flash message to be.

[bartkamphorst]

I could see an argument for providing a customizable macro templates every place we document custom Gollum macros

I'm not entirely sure I follow. :) Is the point that providing one here (ie. Flash(msg)) is overkill? Or that Note and Warn should also take a type argument?

Just to expand on my thinking, I'm operating with a dual aim here. On the one hand, I'm on board with having styling be done as much as possible in gollum. On the other hand, I want to keep it as straightforward as possible for people to add flash banners (with and without icons) to their pages. Preferably, this can be done without utilizing custom.css as not all gollum users are familiar with CSS. So I'm thinking that nice-looking notice and warn flash banners should be easy to create. If, however, someone wants more freedom, they could use this new Flash macro (e.g., <<Flash("<div data-gollum-icon='bell'></div>Important warning here", "warn")>>). (And of course expert users could override the styling entirely in CSS.)

[benjaminwil]

I'm not entirely sure I follow. :) Is the point that providing one here (ie. Flash(msg)) is overkill? Or that Note and Warn should also take a type argument?

I was intending the former, but don't feel strongly at all. I don't think it's "overkill" but I am always a fan of maintaining less code.

I think having a generic, style-able flash message would be great, just curious about what the benefits and tradeoffs are in terms of maintenance, and at what point we should push users to creating their own macros.

[bartkamphorst]

I am always a fan of maintaining less code.

Same here. 👍 I think in this case the code is straightforward and easy to maintain, and even simplified in comparison to what it was before. Of course, we'll get some more code in gollum in return to actually inject the icons.

More generally, when it comes to macros, @dometto and I have been guided both by what other platforms offer and by specific use cases that users have brought to the fore. With use cases, we try to decide whether we see it as feature that would broadly benefit (non-technical, non-programmer) wiki users. If so, we offer it out of the box; otherwise we recommend creating a custom Macro. Does that make sense?

[benjaminwil]

It all makes sense to me! Thanks for your patience and for responding to all my comments. I really don't feel strongly about any of this, and support whatever decisions you choose to make!

[bartkamphorst]

@dometto Any thoughts on this PR in terms of code or in comparison to #440 ?

[dometto]

Sorry to respond to this so late! I like this solution a lot, it's much cleaner than the octicon-entangled situation we had before. :)

Think I found a small issue in the line comments, and also I wonder if this should be merged in to master (current target branch) or 6.x, given that the required changes to CSS (?) in gollum have not been made yet?

[bartkamphorst]

Think I found a small issue in the line comments

What issue are you referring to?

I wonder if this should be merged in to master (current target branch) or 6.x

I guess 6.x is starting to make more sense, especially since people will just lose their octicons if they use master.

[dometto]

I meant the unused optional parameter!

[dometto]

@bartkamphorst looks like the JRuby test failure is trivial: nokogiri on JRuby outputs the attributes for the icon div in a different order than on MRI. Making the test more lenient or explicitly setting a different expecation on JRuby seems like a fine solution. After that this is ready to merge!

[dometto]

Just noticed there are some 'old' commits like this one in this PR. Does a git rebase 6.x get rid of them?

[dometto]

Tests are passing, yay!