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

Generify radar dimension #196

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Generify radar dimension #196

wants to merge 3 commits into from

Conversation

kybishop
Copy link

@kybishop kybishop commented Mar 19, 2018

The overarching goal here is to do the preliminary work necessary for a horizontal-collection component. I figure we would probably pull the data-view files into their own addon, which could then be consumed by horizontal-collection and vertical-collection.

This work is more proof-of-concept than anything else, since we might want to pull this work into said separate addon without merging the PR directly. It's that or rename the addon and include both collections 😉

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

I think at a high level I'm unsure of this approach over a more functional approach, and that we should add automated performance tests before we expand the scope of what Radar handles.

@@ -60,15 +59,15 @@ export default class DynamicRadar extends Radar {
_updateConstants() {
super._updateConstants();

if (this._calculatedEstimateHeight < this._minHeight) {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems odd for _minHeight to still be here in a generalized version

Copy link
Author

@kybishop kybishop Mar 19, 2018

Choose a reason for hiding this comment

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

Still a WIP, will get to this soon. Fixed

@pzuraq
Copy link
Contributor

pzuraq commented Mar 19, 2018

I'm generally onboard with this at a high level. Agree that automated perf tests would be good to add but given that @kybishop is going to need horizontal-collection sooner rather than later, I think we could probably do with manual tests based on ember-macro-benchmark. Do you think you could lay out a plan for automated performance tests if we're going to block on doing this work until we get them?

Also agree that the current strategy of branching and adding measurement functions feels a bit off, though I'm unsure of what a better strategy might be. I could see a functional approach, though it would introduce a fair amount of boilerplate (constantly having to pass in dimension) but it would probably isolate exactly what we are measuring pretty well, so I'd lean that direction as well.

@kybishop
Copy link
Author

kybishop commented Mar 19, 2018

Color me surprised if a functional approach is more performant than set-and-forget on init, though v8 optimization is largely a black box to me, so who knows 🤷‍♂️

It's my understanding that calling properties directly (someBoundingClientRect.top) is much more optimizable than calling by-string (someBoundingClientRect['top']). The only other approach would be to if/else on each function call (return dimension === 'height' ? something.top : something.left), but that seems like a lot more work than just checking the dimension once at init.

I'm very much on board with some perf tests. Would be great to stress test both approaches.

@kybishop kybishop changed the title [WIP] generify radar dimension Generify radar dimension Mar 20, 2018
}

// TODO(kjb) How do we handle this for width? font-size only refers to font height, though we could
// make a rough estimate of width based on some general-case constant.
Copy link
Author

Choose a reason for hiding this comment

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

This should be addressed before moving forward with horizontal collection. It's the only thing I'm somewhat hung-up on since there is no definitive answer.

Copy link
Contributor

@runspired runspired Mar 20, 2018

Choose a reason for hiding this comment

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

font-size applies to width as much as to height

Copy link
Author

@kybishop kybishop Mar 20, 2018

Choose a reason for hiding this comment

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

Maybe this is a stupid question, but then why are there monospaced fonts? It's my understanding that the width of a font is not directly correlated to its height, but arbitrary.

EDIT: Put a bit more succinctly – while font-size might increase width at the same ratio as height is increased, we don't actually know the width of a character for a given font-size since it varies between fonts. Maybe this is a non-issue here; I'm not totally clear on how accurate this estimate needs to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think em and rem font-size is a separate concept from actual rendered font size. If you were to say that em was based on the current font-size of 12px, and then to say that this item was width: 2em, it would be 24px.

@RobbieTheWagner
Copy link
Member

@pzuraq @kybishop where did we leave off on this?

@kybishop
Copy link
Author

kybishop commented Aug 4, 2019

I've been away from Ember for over half a year now. Someone is welcome to take up the mantel on this if they want to, but I definitely don't have the time to jump back into Ember stuff these days.

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.

4 participants