-
Notifications
You must be signed in to change notification settings - Fork 76
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
base: master
Are you sure you want to change the base?
Conversation
619fdc1
to
a2f3c66
Compare
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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 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 |
Color me surprised if a functional approach is more performant than set-and-forget on It's my understanding that calling properties directly ( I'm very much on board with some perf tests. Would be great to stress test both approaches. |
} | ||
|
||
// 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
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 😉