data-background attributes rework #52

obilodeau · 2015-10-06T02:46:31Z

Integrated the work in #38 and the ideas I proposed in #34 for background image support as block image attributes. The PoC is there but the documentation is missing, I'll be waiting on feedback. There are suboptimal and non-rubyish things in there since I'm not familiar with that language.

Breaking changes:

data-background-image is now background referring to the CSS attribute that reveal.js supports
similarily, data-transition, data-transition-speed, data-background-size, data-background-repeat, data-background-transition all lost their data- prefix since it was not required and seen as redundant (see Drop the data- prefix on the slide attributes #38 and Drop the data- prefix on the slide background attributes #18)
:imagesdir: is now honored on background attribute making image handling more consistent

TODO:

in addition to background, canvas should be supported when looking for background images
doc: background image URLs work
doc: background is now relative to :imagesdir: if set
doc: attributes supports whatever CSS supports
document new syntax format
link to https://github.com/hakimel/reveal.js/#slide-backgrounds, https://github.com/hakimel/reveal.js/#slide-transitions
~~link to the relevant moz doc attributes since reveal.js hasn't documented them~~ upstream did it

I pushed my test slides so you can try to reproduce in your environments:

with section attributes: https://github.com/obilodeau/my-asciidoctor-tests/blob/master/data-background-oldstyle.adoc
with new block image background attribute syntax: https://github.com/obilodeau/my-asciidoctor-tests/blob/master/data-background-newstyle.adoc

If accepted, I'll merge Dan's title-slide work and apply the same block_image syntax too.

Edit: added elements to doc TODO

obilodeau · 2015-10-06T02:47:28Z

templates/slim/block_image.html.slim

-  .content
-    - if attr? :link
-      a.image href=(attr :link)
+- unless attr(1) == 'background'


this patch looks larger than it should due to diff being confused. all I did is inject this unless statement.

You can write this as (attr :alt) instead of (attr 1). The first positional attribute is always mapped to the alt attribute for images.

Giving it some more thought, if you stick with (attr 1), then it still would be possible to provide alt text using the alt attribute explicitly. So now I'm inclined to say keep it how it is.

obilodeau · 2015-10-06T02:50:08Z

I forgot backwards compatibility. Any pointers about how to do it without hurting readability too much?

obilodeau · 2016-03-11T20:43:31Z

@mojavelinux do you think the premises for that change are sound? If so, I would finish up the docs and merge this.

As a reminder, have a look at the old markup (that will still work) and the new proposed markup to get a feel of what this PR proposes.

An additional benefit I just noticed is that you see the background images for slides in github's asciidoc preview (since they are image macros instead of hidden away in section attributes).

nipafx · 2016-03-19T06:29:14Z

I did not look deeply into the changes but I like the PR's direction (especially the :imagesdir: inclusion). Any ETA on when this might get merged?

obilodeau · 2016-03-19T12:50:12Z

I'm waiting on the approval of contributors and users.

On Sat, Mar 19, 2016, 2:32 AM Nicolai Parlog [email protected]
wrote:

I did not look deeply into the changes but I like the PR's direction
(especially the :imagesdir: inclusion). Any ETA on when this might get
merged?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#52 (comment)

mojavelinux · 2016-03-20T01:06:02Z

Nice work!

An additional benefit I just noticed is that you see the background images for slides in github's asciidoc preview (since they are image macros instead of hidden away in section attributes).

That's exactly the idea. Visible content should be content. Attributes should modify.

Overall, I'd say it looks good. I tested it and I can confirm it works.

At first, I was a little reserved about overloading the alt text to promote the image to the background. In the Bespoke converter, I'm using a role for this purpose:

image::cover.jpg[Background image,1024,640,role=cover canvas]

or as two lines:

[.cover.canvas]
image::cover.jpg[Background image,1024,640]

Giving it some more thought, I realize that the alt text is usually superfluous and just getting in the way. Taking the approach you are proposing, my example could be simplified to:

image::cover.jpg[canvas,1024,640,size=cover]

That's probably a lot cleaner. I could still provide alt text if necessary:

image::cover.jpg[canvas,1024,640,size=cover,alt=Background image]

I tend to prefer the word "canvas" over "background" since it indicates the layer to which the image is applied as a background. (The canvas being the whole viewport).

So, after letting this sink in, I like the approach. My only change request is to use "canvas" instead of "background", though if you think background fits better for reveal.js users, then I'll let you make that call.

obilodeau · 2016-03-20T14:03:07Z

Since background is the upstream revealjs term I would keep it but I think we can support canvas also. I'll aim for both.

Thanks for the feedback @mojavelinux. It's time to rebase the branch and start ticking off items in the TODO and get this merged.

obilodeau · 2016-03-21T05:02:40Z

Rebase is done. Tasks are up-to-date. I'll be writing the docs in the coming days/weeks.

I don't think we should be concerned about the data-background backwards compatibility breakage at this point. The day we will tag some version then we should start worrying about it. What do you guys think?

mojavelinux · 2016-03-21T10:00:08Z

👍

I don't think we should be concerned about the data-background backwards compatibility breakage at this point.

Agreed.

nipafx · 2016-03-21T14:36:52Z

I like background because

as a non-Web-Developer it is much more obvious to me what it means (compared to canvas)
I appreciate the symmetry to reveal.js' data-background (a symmetry shared by all attributes)

Maybe have both?

obilodeau · 2016-03-21T17:07:06Z

Both: that's the plan

On Mon, Mar 21, 2016, 10:40 AM Nicolai Parlog [email protected]
wrote:

I like background because

as a non-Web-Developer it is much more obvious to me what it means
(compared to canvas)

I appreciate the symmetry to reveal.js' data-background (a symmetry
shared by all attributes)

Maybe have both?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#52 (comment)

kubamarchwicki · 2016-03-29T20:45:05Z

templates/slim/section.html.slim

@@ -1,6 +1,9 @@
 / hide slides on %conceal, %notitle and named "!"
 - titleless = (title = self.title) == '!'
 - hide_title = (titleless || (option? :notitle) || (option? :conceal))
+
+/ slide options processing
+- data_background = (attr? 'background') ? (image_uri(attr 'background')) : nil


What about non image backgrounds? For reveal.js we have an option which doesn't seem supported

<section data-background="#ff0000"> <h2>All CSS color formats are supported, like rgba() or hsl().</h2> </section>

[background="#eee"] == What are we doing?

is currently rendered into

<section data-background="images/#eee"> <h2>What are we doing?</h2> </section>

Great point. In my old patch, I had the following:

- if (bg = attr 'data-background') && bg =~ /\.(bmp|gif|jpe?g|png|svg)$/ - bg_image = image_uri bg - bg = nil - else - bg_image = (attr? 'data-background-image') ? image_uri(attr 'data-background-image') : nil

Agreed. This must definitely go in. I use background colors all the time.
How could I forget...

On Tue, Mar 29, 2016, 4:52 PM Dan Allen [email protected] wrote:

In templates/slim/section.html.slim
#52 (comment)
:

@@ -1,6 +1,9 @@
/ hide slides on %conceal, %notitle and named "!"

titleless = (title = self.title) == '!'

hide_title = (titleless || (option? :notitle) || (option? :conceal))
+
+/ slide options processing
+- data_background = (attr? 'background') ? (image_uri(attr 'background')) : nil

Great point. In my old patch, I had the following:

if (bg = attr 'data-background') && bg =~ /.(bmp|gif|jpe?g|png|svg)$/

bg_image = image_uri bg

bg = nil

else

bg_image = (attr? 'data-background-image') ? image_uri(attr 'data-background-image') : nil

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/asciidoctor/asciidoctor-reveal.js/pull/52/files/e5bb7247eeab4a03bdab87c4705ac650f515b60b..d3534081ef6ce16c36251e98b309a4a38b5b31bb#r57797873

Additionally (as I'm testing my old'ish slide decks with this PR), I was able to setup slides as follows

[data-background="#eee", data-background-image=images/done-part1.gif, data-background-size=cover] == No more Word design docs!

which resulted with a section having a class has-light-background. As per reveal.js release notes (version 3.0.0)

The .reveal element is given a "has-light-background" or "has-dark-background" class when a per-slide background color is detected

Do you have any idea how it's possible now. I'm unable to set up a class attribute on section element, but having a <section class="has-light-background"> will do the trick

kubamarchwicki · 2016-03-29T21:32:00Z

👍 - would like to see that merged. Would be able to drop my hacky personal backend with the background images implementation.

kubamarchwicki · 2016-03-30T07:58:28Z

As for the syntax, can we make it exactly the same as for asciidoctor-bespoke.js Dan is working on? Slides wise I think there will be two engines: reveal and bespoke and making the slides documents as universal as possible asciidoc wise seems like a tempting option

mojavelinux · 2016-03-30T08:24:30Z

I think we agreed on the compatible syntax as:

image::cover.jpg[canvas,1024,640,size=cover,alt=Background image]

The reveal.js converter will also permit "background" in place of "canvas".

I haven't yet implemented this in the Bespoke.js converter.

mojavelinux · 2016-03-30T08:25:12Z

The Bespoke.js converter will also support all the background attributes on the section as soon as that integration is complete. In other words, I'll track reveal.js and try to keep it aligned.

mojavelinux · 2016-03-30T08:26:13Z

At the same time, we should recognize that we are still in the design phase and there will be differences between the converters as we explore new ideas. Eventually, everything will come into alignment as we learn the best way to write the AsciiDoc. I just want to make sure we don't tie our hands down too early.

kubamarchwicki · 2016-03-30T09:31:19Z

On 30 Mar 2016 10:24, "Dan Allen" [email protected] wrote:

I think we agreed on the compatible syntax as:

image::cover.jpg[canvas,1024,640,size=cover,alt=Background image]

The reveal.js converter will also permit "background" in place of
"canvas".

Great! I'll catchup with the docs and examples to get this one merge in.
Least I can do :)

Kuba

More consistent with reveal.js' internals and documentation.

Closes #18

background attribute is no longer guessed to be either a color or an image in reveal.js. Aligned our behavior accordingly.

obilodeau · 2016-08-25T20:35:41Z

This is ready to go in. Can someone acknowledge it please? There are test cases to be rendered in test/ you can use make to run asciidoctor on all of them.

mojavelinux · 2016-08-30T05:06:41Z

templates/slim/section.html.slim

-    section id=(titleless ? nil : @id) data-transition=(attr 'data-transition') data-transition-speed=(attr 'data-transition-speed') data-background=(attr 'data-background') data-background-size=(attr 'data-background-size') data-background-repeat=(attr 'data-background-repeat') data-background-transition=(attr 'data-background-transition')
+    / TODO: try to get rid of duplication w/ standalone slide section
+    section(id=(titleless ? nil : @id)
+      class=[@style,role]


I don't think @style is really needed there. The style of a section is mostly reserved for special sections in DocBook. You can simply write this as:

class=roles

mojavelinux · 2016-08-31T06:06:33Z

test/data-background-newstyle.adoc

+
+== hey
+
+image::bio.jpg[background,size="100px"]


Technically, the quotes around an attribute value are only required if the value has commas or spaces. Just FYI.

mojavelinux · 2016-08-31T06:12:16Z

I added some inline comments. Other than that, I think we're good to go.

mojavelinux · 2016-08-31T06:16:22Z

There's one thing I want to point out, but it's not specific to this change. The attr and attr? methods inherit by default. That means if they don't find the attribute defined on the node, they look on the document.

Generally, you only want to enable inheritance if you intend to allow an attribute of the same name to be controlled globally. That might be good for configuring transitions. For instance:

= My Slides
:transition-speed: fast

== First Slide

However, there may be attributes that you don't want to inherit. If that's the case, you generally use the form:

attr('name', nil, false)

The second parameter value is the default attribute value, which is nil by default.

The text was written by Dan on a pull-request: #52 (comment)

obilodeau · 2016-09-02T01:37:55Z

I added the attribute inheritance note to HACKING.adoc as I am afraid I'll forget. Also did the cleanup. I'm merging this. Thanks!

mojavelinux · 2016-09-02T01:39:12Z

👍

* Removed data- prefixes from all reveal.js features * image block macro now has a background (or canvas) feature that will fill in a background-image attribute for that slide (shorter syntax) * imagesdir attribute is consistently used

obilodeau reviewed Oct 6, 2015
View reviewed changes

obilodeau mentioned this pull request Oct 8, 2015

Add reveal.js jade templates #39

Merged

andreabedini mentioned this pull request Dec 1, 2015

Drop the data- prefix on the slide attributes #38

Closed

obilodeau mentioned this pull request Mar 11, 2016

Add data-state to slides #61

Merged

obilodeau force-pushed the master branch from 692c3f6 to 5349aa8 Compare March 20, 2016 17:32

kubamarchwicki reviewed Mar 29, 2016
View reviewed changes

kubamarchwicki mentioned this pull request Mar 29, 2016

resolves #7 support background options on title slide #34

Merged

mojavelinux mentioned this pull request Apr 14, 2016

uniform use of image_uri #67

Closed

obilodeau and others added 5 commits August 25, 2016 14:38

dropped -image from data-background-image (like reveal.js)

eb9558d

More consistent with reveal.js' internals and documentation.

:imagesdir: is now honored on data-background attribute

5e5ef89

Drop the data- prefix on the slide attributes

e53ec8b

Closes #18

introduced new block_image-based shorter syntax for background images

a880692

data-background rework: Added test cases

40754f7

obilodeau added 3 commits August 25, 2016 15:22

Canvas is also a valid keyword in addition to background (bespokejs)

325e5c8

As per upstream reveal.js background image and color are now explicit

afcac22

background attribute is no longer guessed to be either a color or an image in reveal.js. Aligned our behavior accordingly.

Added documentation and updated test cases

4e0f3f4

obilodeau added the needs review label Aug 26, 2016

Passing classes to slides. Enables easier CSS customization. (fixes #86)

2e5a460

obilodeau mentioned this pull request Aug 29, 2016

role attribute not applied to <section> (slides) #86

Closed

mojavelinux reviewed Aug 30, 2016
View reviewed changes

obilodeau added 2 commits August 30, 2016 11:01

Removed @Style after reviewer suggestion

cc73707

Removed @Style as per reviewer recommendation

0544091

mojavelinux reviewed Aug 31, 2016
View reviewed changes

obilodeau added 2 commits September 1, 2016 21:30

Cleanup based on review comments

474a4cb

Important note about asciidoc's attribute inheritence

1c14be7

The text was written by Dan on a pull-request: #52 (comment)

obilodeau merged commit 1c14be7 into asciidoctor:master Sep 2, 2016

obilodeau removed the needs review label Sep 2, 2016

obilodeau deleted the data-background-rework branch September 2, 2016 03:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data-background attributes rework #52

data-background attributes rework #52

obilodeau commented Oct 6, 2015 •

edited

Loading

obilodeau Oct 6, 2015

mojavelinux Mar 20, 2016

mojavelinux Mar 20, 2016

obilodeau commented Oct 6, 2015

obilodeau commented Mar 11, 2016

nipafx commented Mar 19, 2016

obilodeau commented Mar 19, 2016

mojavelinux commented Mar 20, 2016

obilodeau commented Mar 20, 2016

obilodeau commented Mar 21, 2016

mojavelinux commented Mar 21, 2016

nipafx commented Mar 21, 2016

obilodeau commented Mar 21, 2016

kubamarchwicki Mar 29, 2016

mojavelinux Mar 29, 2016

obilodeau Mar 29, 2016

kubamarchwicki Mar 29, 2016

kubamarchwicki commented Mar 29, 2016

kubamarchwicki commented Mar 30, 2016

mojavelinux commented Mar 30, 2016

mojavelinux commented Mar 30, 2016

mojavelinux commented Mar 30, 2016

kubamarchwicki commented Mar 30, 2016

obilodeau commented Aug 25, 2016

mojavelinux Aug 30, 2016

obilodeau Aug 30, 2016

mojavelinux Aug 31, 2016

mojavelinux commented Aug 31, 2016

mojavelinux commented Aug 31, 2016 •

edited

Loading

obilodeau commented Sep 2, 2016

mojavelinux commented Sep 2, 2016


		== hey

		image::bio.jpg[background,size="100px"]

data-background attributes rework #52

data-background attributes rework #52

Conversation

obilodeau commented Oct 6, 2015 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obilodeau commented Oct 6, 2015

obilodeau commented Mar 11, 2016

nipafx commented Mar 19, 2016

obilodeau commented Mar 19, 2016

mojavelinux commented Mar 20, 2016

obilodeau commented Mar 20, 2016

obilodeau commented Mar 21, 2016

mojavelinux commented Mar 21, 2016

nipafx commented Mar 21, 2016

obilodeau commented Mar 21, 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

kubamarchwicki commented Mar 29, 2016

kubamarchwicki commented Mar 30, 2016

mojavelinux commented Mar 30, 2016

mojavelinux commented Mar 30, 2016

mojavelinux commented Mar 30, 2016

kubamarchwicki commented Mar 30, 2016

obilodeau commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojavelinux commented Aug 31, 2016

mojavelinux commented Aug 31, 2016 • edited Loading

obilodeau commented Sep 2, 2016

mojavelinux commented Sep 2, 2016

obilodeau commented Oct 6, 2015 •

edited

Loading

mojavelinux commented Aug 31, 2016 •

edited

Loading