Repository navigation
Fix #156: Don't scroll upon region change - #165
Conversation
| ); | ||
| } else { | ||
| newState.maybeMetricDescription = ( | ||
| <p className="metric-description" dangerouslySetInnerHTML={ |
There was a problem hiding this comment.
Not sure if it's worth considering something like react-markdownit to avoid using dangerouslySetInnerHTML (which probably calls that API under the hood anyways).
There was a problem hiding this comment.
Nice find! I'll do that in a separate PR since the Markdown stuff isn't modified here.
|
|
||
| if (multipleParagraphs) { | ||
| newState.maybeMetricDescription = ( | ||
| <div className="metric-description"> |
There was a problem hiding this comment.
I checked briefly and didn't see anything suspicious in the CSS, but I assume everything is OK if we have div.metric-description here and p.metric-description below on L54.
There was a problem hiding this comment.
Yep, that should be fine. It's a little weird, but I chose those names intentionally. When there is one paragraph, the metric description is that paragraph / <p>. When there are multiple paragraphs, the metric description is the element (in this case the <div>) which groups those paragraphs.
| </div> | ||
| ); | ||
| }; | ||
| export default MetricOverview; |
There was a problem hiding this comment.
Why the switch to exporting a named class? Do we have more than one?
There was a problem hiding this comment.
My understanding is that, because getDerivedStateFromProps is static, the variables it references must also be static. For example, it references the static instance variable MetricOverview.markdownParser here:
I don't think there's a way to reference a static variable in an unnamed class, but let me try again.
There was a problem hiding this comment.
I almost found a way to do this that works, but it has some weird side effects. For example, in MetricOverview.js, I'd need to refer to metricParser by the name this.a.metricParser, which is really strange. I have to imagine it's a result of babel doing something weird.
Definitely open to making this better. I just can't figure out how.
There was a problem hiding this comment.
Sorry should've mentioned this is a style nit. If it takes more than 8 bytes to address it - ignore it. Was more for consistency with other components.
There was a problem hiding this comment.
Nah don't be sorry. I'm totally on board with you. I wish I knew of a way to do this without naming the class but I can't find a good solution to that. It may be my inexperience with static ES6 methods.
There was a problem hiding this comment.
Style matters. I really believe in that. Just be glad I didn't set up a mandatory pretty-printer for this project like I have in the past. 😜
Although prettier does look very cool.
| }); | ||
| } | ||
|
|
||
| if (Object.keys(newState).length > 0) return newState; |
There was a problem hiding this comment.
In marketplace we used to require curly brace blocks (even when ugly) for better uglifyjs support. I imagine things have improved since then though. Also could be just .length and still be truthy but this is a nit...it might be more readable as is.
There was a problem hiding this comment.
Good catch. I'll fix the .length thing. Do you have a link to info on the uglify thing? I don't always like writing to implementation but if it's still a distinction Uglify makes we could do it.
| class MetricOverview extends React.Component { | ||
| // Create a markdown parser that only parses links. It needs to be static | ||
| // for static methods to use it. | ||
| static markdownParser = markdownIt('zero').enable('link'); |
There was a problem hiding this comment.
I wonder is JIT cares about static and whether it helps performance. Otherwise this reminds me of CSS' !important breeds more !important.
There was a problem hiding this comment.
Good point. And a valid comparison. Unfortunately getDerivedStateFromProps has to be static so I don't think we have another choice. At least not one that I know of, but I'm open to suggestions. I'm new to static methods in ES6.
This commit ended up being a little more involved than expected. This bug was happening because the MetricsGraphics component was re-loaded every time the region selector changed. Fixing the bug meant only loading the MetricsGraphics component once, which meant responding to state changes a little more intelligently. So a lot of state-management code was changed. The good news is that I took the opportunity to improve performance a bit. For example, chart descriptions are no longer re-rendered every time chart data changes.
|
Updated and rebased. You might want to check extra carefully that I didn't mess up any of your chart resizing stuff. I'm pretty confident I didn't, but the rebase involved some of those changes. |
|
r+ thanks |
3 participants
This commit ended up being a little more involved than expected. This
bug was happening because the MetricsGraphics component was re-loaded
every time the region selector changed. Fixing the bug meant only
loading the MetricsGraphics component once, which meant responding to
state changes a little more intelligently. So a lot of state-management
code was changed.
The good news is that I took the opportunity to improve performance a
bit. For example, chart descriptions are no longer re-rendered every
time chart data changes.