Integrate css blocks #580

Merged
merged 18 commits into from Dec 12, 2016

Projects

None yet

2 participants

@dwsmorris
Contributor

This is the code behind proposal #579.

Styling is possible, and even quite pleasant, directly through Eve. However, as mentioned here it cannot easily replace CSS in areas like pseudo classes, animations, and media queries.

So being able to integrate a styling solution into Eve documents soon becomes pretty essential.

Thanks to Corey for fixing #554. It's a one-liner but I just couldn't manage to find it - and it's required for this support.

The CSS from active blocks is directly injected into the document. There were some minor changes required to support other types of code blocks, and allow them to be disabled.

@joshuafcole joshuafcole self-assigned this Nov 21, 2016
src/ide/spans.ts
@@ -617,7 +617,7 @@ export class CodeBlockSpan extends BlockSpan {
protected footerWidgetElem:HTMLElement;
apply(from:Position, to:Position, origin = "+input") {
- this.lineBackgroundClass = "code";
+ this.lineBackgroundClass = "code" + ((this.source.info && this.source.info.toLowerCase().indexOf("css") === 0) ? " css" : "");
@joshuafcole
joshuafcole Dec 1, 2016 Contributor

There's an interesting question here of whether or not blocks of other languages may be disabled. Since CSS has no stateful side effects, it seems like a good candidate for supporting that.

Either way, it seems like it might be nice to abstract the info parsing logic into it's own set of functions, instead of doing it ad-hoc on a per-syntax basis. a syntax():string method returning the syntax portion of the info if present would be a good start.

src/ide/spans.ts
@@ -649,7 +649,7 @@ export class CodeBlockSpan extends BlockSpan {
disable() {
if(!this.disabled) {
- this.source.info = "eve disabled";
+ this.source.info = (this.source.info ? this.source.info : "eve") + " disabled";
@joshuafcole
joshuafcole Dec 1, 2016 Contributor

With the syntax() guy this logic becomes a little cleaner in enable() and disable() since it no longer requires a replace()

src/runtime/runtimeClient.ts
+ css += g1;
+ });
+
+ document.getElementById("app-styles").innerHTML = css;
@joshuafcole
joshuafcole Dec 1, 2016 Contributor

The runtimeClient is sort of unfortunately-named. It is the client for the runtime, but it lives wherever the the runtime is executing, which may be the server. Because of this, you can't reliably access document here. You'll need to send the parsed CSS to the actual client as part of the parse message and have the client update the styles.

@joshuafcole
joshuafcole Dec 1, 2016 Contributor

Hardcoding that explicitly (as I suggested) has a bit of a code smell that our current planned extensibility story doesn't fix. I don't know that it's worth blocking the PR over, but it's probably worth making an issue about.

css/ide.css
@@ -119,6 +119,7 @@ body { justify-content: stretch; }
.CodeMirror .STRONG { font-weight:bold; }
.CodeMirror .EMPH { font-style:italic; }
.CodeMirror .CODE { margin-left: 10px; margin-right:50px; border-left:5px solid #eaeaea; background: #f4f4f4; font-family: "Inconsolata", "Monaco", "Consolas", "Ubuntu Mono", monospace; }
+.CodeMirror .CODE.css { background: #fff7e9; border-left-color: #ffedcc;}
@joshuafcole
joshuafcole Dec 1, 2016 Contributor

The style here could probably use a little work, though I don't know what the right differentiator would be. The current color is aesthetically nice but is reminiscent of a warning highlight.

css/ide.css
@@ -176,7 +177,7 @@ body { justify-content: stretch; }
* Format Bars
\****************************************************************************/
.editor-pane .new-block-bar { position: absolute; top: 0; left: 0; margin-left: -28px; display: flex; flex-direction: row; width: 28px; overflow: hidden; transition: width 0.15s; z-index: 5; color: #666; border-radius:5px; border:1px solid #ddd; font-size:10pt; background:white; }
-.editor-pane .new-block-bar.active { width: 246px; }
+.editor-pane .new-block-bar.active { width: initial; }
@joshuafcole
joshuafcole Dec 1, 2016 Contributor

Unfortunately initial doesn't work here, it breaks the transition. It galls me too that this has to be hard-coded, but I don't know of any other reasonable path short of a whole lot of silliness involving wrapping elements and transforms.

@joshuafcole
Contributor
joshuafcole commented Dec 1, 2016 edited

Hey @dwsmorris! Thanks for the PR, and apologies about the delayed review.

This is something we'd been meaning to experiment with, so we're interested in getting this merged. I've added a couple suggestions on the code itself, but there are also a few global items:

  • It'd be very nice if we can find a way to enable CodeMirror's built in CSS syntax highlighting within the CSS block.

  • It'd also be nice to limit the CSS block to only impact the .application-container > .program's children. We could do this through a selector rewrite. Otherwise users have to be very careful about specificity since div { background: red; } will trash the editor.

  • For non-eve codeblocks, it would be nice to embed the block syntax as a title in the block's top padding, or somewhere else visible.

Again, thank you for this! I look forward to working with you to get it in. :)

@dwsmorris
Contributor

Hi Josh,

Fantastic! I'll try and get these changes done this weekend.

Cheers,
Darryl

@dwsmorris
Contributor

cssblocks

  • Changed the colour of css blocks to blue
  • Example app css is restricted to application container
  • CodeMirror css styling is applied. If we want to support blocks from additional languages, then it may require a more invasive refactor. Single-language syntax highlighting seems to be applied uniformly over the entire document (even creating different CodeMirror sections, one for each separate code block, may not make it easy).

placement

  • I adjusted the position of the checkbox slightly on code blocks, and made the top padding of code blocks very slightly larger. This is to prevent overlapping of the code with the checkbox, as seen above. Though unlikely in Eve blocks, CSS lines can get long and so it may have been an issue.

Many thanks for the pointers - let me know any further suggestions/improvements that come to mind.

@joshuafcole
joshuafcole requested changes Dec 5, 2016 edited View changes

Little bit more work required for CSS selector prefix injection, but it's nearly there and looking great!

src/runtime/runtimeClient.ts
+ this.lastParse.code.replace(/(?:```|~~~)css\n([\w\W]*?)\n(?:```|~~~)/g, (g0, g1) => { // \n excludes disabled blocks
+ css += g1;
+ });
+ css = css ? css.split("\n").map(function(line) {
@joshuafcole
joshuafcole Dec 5, 2016 Contributor

Unfortunately not everyone likes to write their CSS with a single line per selector, so we need to handle two other major cases:

.foo .bar {
  background: red;
  color: green
}

and

.foo .bar
{
  background: red;
  color: green
}

I think you can hit all 3 with the following rules:

  1. The line begins with a non-brace and contains an opening brace
  2. The line begins with a non-brace and is followed by a line containing an open brace prior to a line containing a closing brace.

Once you've matched just selector lines, you also need to split those on commas and inject the selector prefix on each chunk.

src/ide/spans.ts
+
+ syntaxHighlight() : string {
+ // provide codemirror syntax highlight indications for css blocks only
+ return this.syntax() === "css" ? "cm-s-default" : "";
@joshuafcole
joshuafcole Dec 5, 2016 Contributor

We should look into using CodeMirror's overlay mode to enable hypothetical other languages in the future [1]. This is a want right now, not a need. Take a look to see how long you think it would take. If it seems significant we can instead drop an // @TODO: ... in the code and revisit it when it becomes relevant.

[1] https://codemirror.net/demo/mustache.html
[2] https://codemirror.net/addon/mode/overlay.js

@dwsmorris
Contributor

Great - I'll have some time to look at this on Thursday.

@dwsmorris
Contributor

I had a look at the CodeMirror overlay mode example. It demonstrates augmenting an existing global syntax-colouring scheme with a new rule - possibly a start, but partitioning the application of existing colouring rules on a per-block basis may still be a fiddle - so I added a comment in the code for now. (I'll keep looking into it when I can get some time if no-one else fancies taking it on. I'm interested to see if switching on-and-off reactive javascript blocks can actually work.)

I updated the css rewriting code. There were further complications in supporting media queries (where you could have multiple opening braces on a single line). I've tested the scenarios you raised and now also support blocks like:

@keyframes example {
    from {background-color: red;}
    to {background-color: yellow;}
}
@media screen and (min-width: 480px) {
    .my-div {
        background-color: lightgreen;
    }
}
@keyframes mymove {
    0%   {top: 0px;}
    100% {top: 200px;}
}

This code may have to be incrementally improved when someone finds it doesn't handle some aspect of CSS correctly.

@joshuafcole

There's one other small issue, I can fix it after the merge if you're strapped for time. It's not the fault of any of your code, but the issue was exposed here since the CSS blocks are created differently. In some cases The intermediate CSS block gets cleared and replaced after already having been cleared. the clearWidgets() method doesn't check for this case, so blows up.

src/ide/spans.ts#L723 ->
if(!this.widget) return; covers that case.

+ css = css.split("\n").map(function(line) {
+ var trimmedLine = line.trim();
+ if ((line.indexOf("{") !== -1) && // selector line
+ ("0123456789@".indexOf(trimmedLine[0]) === -1) && // no animation percentages or @-rules
@joshuafcole
joshuafcole Dec 10, 2016 Contributor

Instead of bailing if the selector contains a number (which may be a valid character in a selector), let's bail if it contains a %, which never is. MDN specifies the % is required for keyframes, so I think that should have us covered.

@dwsmorris
dwsmorris Dec 11, 2016 Contributor

I only bail here if the selector starts with a number (checking first character only), which selector grammar doesn't allow - so I think it should just exclude animation percentage specifications and not other selectors.

@joshuafcole
Contributor

Looks great. Lets get this merged tonight so we can include it in release 0.2.3, which is due early next week.

Thanks again for time, I think people are really going to enjoy this. :)

@dwsmorris
Contributor

I added your fix in clearWidgets();

Really glad you decided to go for this change!

Thinking again about your comment on runtimeClient.ts#L110, the intent may have not been clear. Your suggestion may be clearer, in which case we could substitute the line for something like:

          (trimmedLine[0] !== "@") && // no @-rule
          (trimmedLine.indexOf("%") === -1) && // no animation percentages

Because newlines are inserted after every "{", we shouldn't have to worry about valid "%" signs (in the keyframe value) being on the same line.
In any case, if you think it conveys intent better, I can apply, or please apply after merge.

@joshuafcole
Contributor

Your approach makes sense to me and is likely more performant. In skimming the spec I'd missed that selectors couldn't start with numerals. When I get home I'll get this merged and then pull it into the 0.2.3 branch, which will go live on play.witheve.com and npm next week.

@joshuafcole joshuafcole merged commit f57adbc into witheve:master Dec 12, 2016
@joshuafcole
Contributor

Thanks again for sticking with it! Keep an eye out next week for 0.2.3. :)

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