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

First early version #7

Merged
merged 40 commits into from
Feb 16, 2017
Merged

Conversation

hsablonniere
Copy link
Collaborator

@hsablonniere hsablonniere commented Feb 8, 2017

This is a proposition for a 0.1.0 version. It addresses #3, #4, #5 and #6.

Content of the PR

  • An MIT license
  • A new README
    • Do we need the details about how notes are authored? If we do, we'll also have to think about deploying the bespoke plugin and explain that this tool only works with bespoke (for now).
  • A globally installable command line tool prz (instead of the npm/yarn script)
  • Some file cleanups (I removed a lot of files that are WIP, there are still in a branch, we'll use them later)

NOTE: Some renaming will be needed between components and the names we use in the doc (presentation console & projector viewport).

TODO (after merge)

Copy link
Contributor

@mojavelinux mojavelinux left a comment

Choose a reason for hiding this comment

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

Can we dim the next slide? Right now it's hard to distinguish the state of the two slides.

I think we should put a placeholder word when a slide has no notes, which we can do with CSS. Otherwise, I find it confusing what the cursor is pointing to. Something like "(no notes)" or "-" would work.

.notes-line:empty::after {
  content: '(no notes)';
  font-style: italic;
  opacity: 0.33;
}

I don't think we should highlight the notes on hover, or at least not so prominently. Perhaps place a border or outline to indicate the boundaries, but the dark background color is confusing.

I think the text in the notes viewer needs to be a little tighter. Here are the settings I propose:

  • font-size: 28px
  • padding: 0.5em 1em;
  • hanging indent: 1.25ex

I think we should hide the scrollbar on Chrome / IE using browser-specific styles. See https://github.com/opendevise/presentation-bespoke-starter/blob/master/src/styles/bespoke-modes.styl#L2-L3

I'd like to have a button to hide the timer bar, but that can be part of a follow-up release.

README.adoc Outdated

This project is a Work In Progress
_ensuite-present_ is part of the *ensuite* toolbelt dedicated to trainers and speakers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call it a toolkit.

README.adoc Outdated

== Overview

_ensuite-present_ is a local Web app that allows an instructor to "open" and control a slide-deck.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's lowercase Web here. There are different opinions, but I think it makes it more readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to try :trollface:

README.adoc Outdated

To run _ensuite-present_ on your machine, you just need to run this command:

$ prz
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I keep going back and forth on this, but I wonder if we should name it prez, es-prez, or es-present instead of prz. Seeing it alone here kind of gives me a feeling of wanting to change the name. We don't have to get it right for this release, just something to think about before 1.0.0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would defer that to another release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to defer. My brain is really getting attached to "enpres" though. But no need to rush it. We have time to let it soak.

const http = require('http')
const nodeStatic = require('node-static')

const PORT = 4242
Copy link
Contributor

Choose a reason for hiding this comment

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

4242 is the port used by Awestruct. I think we should use 4320 instead (since we're proposing 4321 for the bespoke generator).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I still think bespoke should propose a pseudo-random port but that's another disussion 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll make 4320 configurable in later releases.

const fileServer = new nodeStatic.Server()

http
.createServer((request, response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

When installed, this seems to be serving files from the bin directory instead of from ensuite-present.

Copy link
Contributor

@mojavelinux mojavelinux Feb 13, 2017

Choose a reason for hiding this comment

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

We need something like:

const fileServer = new nodeStatic.Server(path.dirname(path.dirname(require.resolve('ensuite-present'))))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I implemented it like this:

const webRoot = path.resolve(__dirname, '..')
const fileServer = new nodeStatic.Server(webRoot)

@mojavelinux
Copy link
Contributor

Do we need the details about how notes are authored?

Not at this time. Perhaps just make a brief reference to https://github.com/asciidoctor/asciidoctor-bespoke#speaker-notes and that more info is coming.

@mojavelinux
Copy link
Contributor

I think we should simplify the page URLs. Right now it's:

  • pages/slide-deck-viewer/slide-deck-viewer-page.html
  • pages/teleprompter-console/teleprompter-console-page.html

I think this could be simplified to:

  • pages/viewer/index.html
  • pages/console/index.html

I also think the query string parameter should be shorten from slide-deck-url to load or deck or url.

@mojavelinux
Copy link
Contributor

Another idea for the tool names is to prefix them with "en" and replace "suite" with the name of the tool. For example, "enpres". Roughly translated, that means something related to a business venture in some languages, which is somewhat interesting.

I may be overthinking this. Names tend to create that problem.

@mojavelinux
Copy link
Contributor

Okay, I see that ex and ch are different, but can act the same depending on the font (if x height matches 0 width, which is true in some cases). I'm 100% in support for ch for this usage.

@mojavelinux
Copy link
Contributor

I like the new page names and the router. Perfect.

I see more than one console in the future so I added teleprompter.

Once we get to that point, we can certainly rename pages. As you said, the user goes to where we tell them to go, so it's not a huge deal to rename pages in the future. And when the use case comes up, it should be clear what names we need to use.

@mojavelinux
Copy link
Contributor

I made a small update so that the left border on the notes does not show for the current notes since they are already highlighted in yellow.

I also changed to the more subtle VIM-style no content. (Just a tilde).

@hsablonniere
Copy link
Collaborator Author

I rewrote history to remove a forgotten comment in the VIM style commit.

@hsablonniere
Copy link
Collaborator Author

I think we'll have to improve implementation on:

  • The "router" obviously
  • The __dirname try/catch

@hsablonniere hsablonniere merged commit 73acfb5 into ensuite:master Feb 16, 2017
@hsablonniere hsablonniere deleted the first-early-version branch February 16, 2017 15:42
This was referenced Feb 16, 2017
@mojavelinux
Copy link
Contributor

I agree the calculation of the web root is not pretty. However, it currently allows the script to work in dev mode (from inside the repository) or installed mode (installed as a bin script with a loadable module). If there's a way to do this correctly in the node/npm ecosystem, I'd love to know. However, I could not find a way other than what I came up with. The catch will never be hit in installed mode, so for me that's pretty safe.

Btw, it's typical to use this trick in the Ruby ecosystem, so for me it's natural. One other approach is to try to detect if the script is relative to another known file. If so, we know what mode we're in.

@mojavelinux
Copy link
Contributor

My statement about ch units above was incorrect. I was a victim of my own optical illusion. As it turns out, the characters on different lines are, in fact, lining up correctly.

@mojavelinux
Copy link
Contributor

I looks like we're still missing a tag in the repository. Can you add it?

@hsablonniere
Copy link
Collaborator Author

hsablonniere commented Feb 17, 2017

😱 Oops, I pushed it to my forked instead of this remote.

https://github.com/ensuite/ensuite-present/releases/tag/v0.1.0

@hsablonniere
Copy link
Collaborator Author

I agree the calculation of the web root is not pretty. However, it currently allows the script to work in dev mode (from inside the repository) or installed mode (installed as a bin script with a loadable module). If there's a way to do this correctly in the node/npm ecosystem, I'd love to know. However, I could not find a way other than what I came up with. The catch will never be hit in installed mode, so for me that's pretty safe.

We'll hack a look later. As we said many times (or will say): "make it until your improve it!".

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

Successfully merging this pull request may close these issues.

2 participants