FEATURE: Poll breakdown 2.0 (PR #10345)

The poll breakdown modal replaces the grouped pie charts feature.

image

image

image

The PR includes a fix/change to d-modal (https://github.com/discourse/discourse/commit/b7f6ec60c5a7185b504048e0fa169f2c23865db0) that hasn’t been extracted to a separate PR because it’s not currently possible to test a change like this in abstract, i.e. with dynamically created controllers/components in tests. The percentage/count toggle test for the poll breakdown feature is essentially a test for that d-modal modification.

GitHub

One thing you could do here is put them in the declaration with null as the value. Javascript likes this actually, because it declares the “shape” of the object which can have better performance.

I’ve provided some fairly minor feedback, because overall this looks great. I wouldn’t mind if @jjaffeux took a look at it too and provided input.

Since we’re unlikely to observe or make a CP based on chart you might as well say this.chart = instead of set.

You could also consider giving _chart an underscore to indicate it’s private unless you expect other people to use it in their code outside of this class.

I prefer absolute paths in our app rather than relative because I find it makes copying/pasting and searching easier

This could use propertyEqual

This could use Ember’s equal macro.

can we use this here https://api.emberjs.com/ember/release/functions/@ember%2Fobject%2Fcomputed/mapBy ?

I know this is not always easy to do, but generally speaking I find set inside a computed to be risky, can we avoid this ?

should probably be defined in init

setProperties ?

this is default so not much needed but :man_shrugging:

do we use rem elsewhere in the app ?

It looks very good, both visually and code wise :1st_place_medal:

How does it look on mobile?

Yeah, I’ve renamed that property to _optionToSlice and since it isn’t a tracked prop, I changed it to just this._optionToSlice = {}. It basically only acts as a cache, so another method can use this without recomputing.

*grumble* *grumble* *native classes* _optionToSlice = {}; *grumble* :wink:

changed :stuck_out_tongue:

I’m actively avoiding setProperties to make it easier for future-me to update all this.set(x, y) calls with Octane’s this.x = y :grin:

Yeah, here and there. But it’s not used as much as I’d like to. :wink:

I prefer rem over em for use in components because it better isolates them from the context they’re being embedded into. For example, if I add a share button to some paragraph that happens to use a smaller font size, I wouldn’t expect it to be smaller but rather exactly the same as anywhere else in the app.

How does it look on mobile?

It doesn’t (https://github.com/discourse/discourse/pull/10345/commits/9b6dae7b910def9b4849fe5a75625383e9555bb1) :trollface:

I’d love to eventually enable it for mobile but it will require coming up with a solution for displaying the legend in a readable way, since a two-column layout won’t do on smaller screens. Anyway, that’s also sometimes an issue for polls on mobile in general. I have some ideas so I’ll circle around to it.

I found 11 places for now, so rather not used :stuck_out_tongue:

We should talk of this with @awesomerobot, I generally prefer rem too and wish we would use it more, but… :stuck_out_tongue: