Allow custom hud positioning

[ivanvoznyakovsky]

add possibility to position the hud in all 4 corners of the screen

[pkaminski]

Thanks for the PR! I like the idea, but would like to tweak the implementation a bit:

1. Instead of passing the position to enable(), could you make it another configurable setting, like numTopWatches?
2. Instead of cryptic shorthand like br, let's use full phrases like bottom right. This will also simplify your parsing, since you can just split the position setting right into the CSS properties to set to zero.

Also, can you mention the config in the README so others can find it? Thanks.

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io

[ivanvoznyakovsky]

Instead of passing the position to enable(), could you make it another configurable setting, like numTopWatches?

I did it like that from the beginning, but then you should set position param before you call enable otherwise it would be undefined. What I would really do is make a directive out of it ))))

Instead of cryptic shorthand like br, let's use full phrases like bottom right

sure, reasonable

will add notes to README

[pkaminski]

I did it like that from the beginning, but then you should set position param before you call enable otherwise it would be undefined.

I don't think so -- just like numTopWatches, you can set this.position = 'bottom right'; as the default to remain backwards-compatible.

---

Comments from the review on Reviewable.io

[ivanvoznyakovsky]

sure, but after you set it after calling enable() it'll not change position cause this.position is not being watched for changes. I meant that to make it work as it is rendering now you need to setup position setting only before calling enable()

[pkaminski]

But you're only supposed to call enable() at most once anyway (I just realized I'm not enforcing that, oops), so you're not losing anything by forcing the setting into the once-only config phase...

[ivanvoznyakovsky]

once only is not the problem. the problem (or not?) is to force user to set position option before calling enable and never after cause it won't work.

[ivanvoznyakovsky]

how about that?

[pkaminski]

Reviewed 2 of 2 files at r2. Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

^{digest-hud.js, line 342 [r2] (raw file):} Could you just split(/\s+/) instead?

---

^{digest-hud.js, line 351 [r2] (raw file):} What if position.length > 2, or you get a position like top bottom? I think validating these is a rathole, especially for a low-level development library like this. I'd be tempted to just split the given string and set any token it contains to 0. And if you are going to validate, it's probably worth throwing an error if something's wrong.

---

^{digest-hud.js, line 357 [r2] (raw file):} hudElement may not be set yet. You need to allow for setHudPosition to be called either before or after the enable. Also, I now get your original concern about ordering of the call, and would be OK with putting the position in as an argument to enable like you had originally.

---

^{README.md, line 9 [r2] (raw file):} Make clear this is optional.

---

Comments from the review on Reviewable.io

[pkaminski]

LGTM, thanks!

---

Reviewed 2 of 2 files at r3. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io

[ivanvoznyakovsky]

would you pls update bower package )))

[pkaminski]

Dammit, I forget every. single. time. Done!

On Thu, Jul 16, 2015 at 4:45 AM, Ivan Voznyakovsky <notifications@github.com

wrote:

would you pls update bower package )))

— Reply to this email directly or view it on GitHub https://github.com/pkaminski/digest-hud/pull/6#issuecomment-121933171.

Piotr Kaminski piotr@ideanest.com "That bun is dirty. Don't eat that bun."