Skip to content

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 😉

@kybishop kybishop force-pushed the generify-radar-dimension branch from 619fdc1 to a2f3c66 Compare March 19, 2018 08:08
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.

_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.

5 participants