Insights: revamp dashboard design, add locale selector#4296
Conversation
functionzz
changed the title
|
Based on the name, this sounds related to #4265, but isn't, right? |
|
Correct, it is not related to #4265. It is more generally related to design updates for the Insights Dashboard. |
functionzz
changed the title
Since it requires further discussion, we should file a separate issue for this and remove the implementation from this PR. |
16e8145 to
a3cc686
Compare
functionzz
changed the title
mathjazz
left a comment
mathjazz
left a comment
There was a problem hiding this comment.
Nice work! Left some comments inline.
One more thing: it turns out the "Translators" table header is too long, because the sorting arrow overflows the cell when it appears. Can you shorten it to "Transl." and show a tooltip with the full name on hover?
| .toggleClass('fa-chevron-left', isHidden); | ||
|
|
||
| if (!isHidden && selectorChange) { | ||
| saveCommunityHealthLocales(true); |
There was a problem hiding this comment.
If the locales are saved instantly on each change in the selector, why do you save them again here?
There was a problem hiding this comment.
Because saveCommunityHealthLocales also controls table regeneration if renderTable = true.
There was a problem hiding this comment.
Right, and what I'm trying to say is that this feels odd. We should have two separate views to handle to separate actions.
|
I removed $(function() {...}); not sure if you want to keep it defensively @mathjazz |
mathjazz
left a comment
mathjazz
left a comment
There was a problem hiding this comment.
Sorry, looks like I forgot to include these two comments somehow.
| .toggleClass('fa-chevron-left', isHidden); | ||
|
|
||
| if (!isHidden && selectorChange) { | ||
| saveCommunityHealthLocales(true); |
There was a problem hiding this comment.
Right, and what I'm trying to say is that this feels odd. We should have two separate views to handle to separate actions.
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
| }); | ||
| } | ||
|
|
||
| $(function () { |
There was a problem hiding this comment.
Why did you add it back?
There was a problem hiding this comment.
So it seems $('body').on('click', '.multiple-item-selector .item.select li', function () { requires delegation.
I added it back to solve that. Now this time I brought all the other handlers to be direct binded, except the ones related to the multiple item selector.
…switch up delegation
|
|
||
| function renderCommunityHealthPanel() { | ||
| $.ajax({ | ||
| url: '/insights/ajax/render-table/', |
There was a problem hiding this comment.
Sorry, didn't notice this earlier. It's not just the table, it's also the chart:
| url: '/insights/ajax/render-table/', | |
| url: '/insights/ajax/render-panel/', |
Please also update the URL pattern name and the view name.
| Chart.getChart('community-health-chart')?.destroy(); | ||
| $('.community-health-score-container').html(response.html); | ||
| Pontoon.insights.renderGlobalChart($('#community-health-chart'), 'chs'); | ||
| Pontoon.endLoader('Locales updated.'); |
There was a problem hiding this comment.
Actually, now we have to drop this line. If you switch between the panel and the locales config, you always get this message, even if you don't make any changes.
There was a problem hiding this comment.
Yes, I believe that was the original reason I had the selectorChange variable
4 participants
This PR makes the following changes:
/insightsThis PR adds the locale selector for the community health dashboard and chart directly into
/insightsand removes the/insights/configpage in favor for a dynamic form solution. It also follows the updated dashboard design for community health score as follows:It also changes the dashboard design to be more consistent with the implementations of the global graphs, by merging the dashboard with the community health graph and changing some button positioning:
Discussion is needed on whether we should switch to a live model for the current dashboard display, with CHS used for the month-over-month differential and the graph (it is expensive to calculate values for the tooltip, why not just extend it for the current month display?)Fixes #4271.