Frontend changes for showing tabbed course overview stats #5064

vaidehi44 · 2022-07-21T20:19:11Z

What this PR does

This PR contains frontend changes corresponding to tabbed overview stats presented on course overview page.

The structure for course_stats object is -

{
  'www.wikidata.org' => WIKIDATA_STATS,
  'en.wikipedia.org-namespace-0' => ENWIKI_MAINSPACE_STATS,
  'en.wikibooks.org-namespace-102' => WIKIBOOKS_COOKBOOT_STATS
}

Screenshots

If the wikidata stats component is used in course overview tabs, there the extra padding and border is not required. But, it is necessary if the component is displayed on campaign overview page.

ragesoss · 2022-07-22T15:02:19Z

I think there's some room for design improvements for the tabs. I'm still unsure whether showing all the tabs like this would be better than have a dropdown to select between them, but here's the design (by Toqa, a designer I hired to work on it a few months ago) that we have for it:

ragesoss · 2022-07-22T15:03:42Z

app/assets/javascripts/components/common/OverviewStats/namespace_overview_stats.jsx

+ renderZero={true}
+ />
+ <OverviewStat
+ id="student-editors"


I don't think the user count needs to be in any namespace-specific stats.

Won't it give insight of how many students worked on particular tracked namespace?

ragesoss · 2022-07-22T15:06:56Z

app/assets/javascripts/components/common/overview_stats_tabs.jsx

+const OverviewStatsTabs = ({ statistics }) => {
+ const [currentTabId, setCurrentTabId] = useState(0);
+
+ const getTabTitle = (ns_id, wiki) => {


These two functions should probably be in a utils file rather than in this component. Tests for them would also be good.

ragesoss · 2022-07-22T15:10:53Z

app/assets/javascripts/components/common/overview_stats_tabs.jsx

+ const project = wiki.split('.')[1];
+ let ns_title = ArticleUtils.NamespaceIdMapping[ns_id];
+ if (typeof (ns_title) !== 'string') ns_title = ns_title[project];
+ return `Stats for ${I18n.t(`namespace.${ns_title}`)} (${wiki})`;


This message shouldn't be hard-coded in English. Per the design image I shared, we might not need a message like this anyway; the label for each tab could just be the domain plus the namespace name (when there is one).

ragesoss · 2022-07-22T15:15:57Z

app/assets/javascripts/components/common/overview_stats_tabs.jsx

+ };
+
+ const statsDataList = [];
+ // if there are wikidata overview stats, they should be displayed on first tab


This and the forEach loop are more data manipulation than I'd like to see. If possible, it would be best to do this in a way that doesn't special-case Wikidata. (I don't think it necessarily needs to be first.)

Also, I got this conceptual question. For each tab we have a different content rendered by different component. So, is it better to create this components, store and use them (like I am doing now), or is it better to initialize and render them instantaneously when a tab is selected?

I think in our case, the compoents are not that heavy and so they can probably be stored before hand.

ragesoss · 2022-07-22T15:22:50Z

...mponents/students/components/Articles/SelectedStudent/RevisionsList/StudentRevisionsList.jsx

 import studentListKeys from '@components/students/shared/StudentList/student_list_keys.js';

 export class StudentRevisionsList extends React.Component {
 constructor(props) {
 super(props);
 this.state = {
 isOpen: false,
- namespace: 'all'
+ namespace: -1 // show all namespace revisions by default


For a code comprehensibility perspective, I don't think replacing strings with numbers here is helpful. Do the changes here have any effect on behavior? If they are unrelated to displaying stats on the Overview tab, they probably shouldn't be included in this PR.

These changes are related to the namespace filter of revisions table.
Actually, earlier this code used namespaceToId function from ArticleUtils. But, now that I have defined new mapping function there (NamespaceIdMapping), that function would have become its duplicate. Hence, some changes reflected here.
Should I open another pr then?

I think this is okay, except... is there any reason to use -1 in place of all? The other integer values all map to a mainspace, but this one does not and -1 not as easy to interpret as all.

Actually, I don't know what number to use for 'all', because for other options it would be their namespace id. May be, we can still use the strings - 'all', 'article', 'talk', etc.

I think a mix of numbers (for actual namespaces) and a string for all would be okay. But also, using strings for all of the values seems find to me.

ragesoss · 2022-07-22T15:30:29Z

I think the namespace titles displayed in the tabs could be something like: en.wikibooks.org - Cookbook, en.wikipedia.org - Mainspace and www.wikidata.org.

It would be good to treat the titles in parallel, so that whatever parsing of the stats hash key is used, it works the same way for keys that include a namespace and those that don't. (ie, it should be able to handle www.wikidata.org, www.wikidata.org-namespace-120, en.wikibooks.org, and en.wikibooks.org-102.)

vaidehi44 · 2022-07-22T18:24:42Z

I think there's some room for design improvements for the tabs. I'm still unsure whether showing all the tabs like this would be better than have a dropdown to select between them, but here's the design (by Toqa, a designer I hired to work on it a few months ago) that we have for it:

I guess, right now the only reason for not having tabs would be because they can extend horizontally beyond page. But, if it is handled and made scrollable or something like that, I would prefer tabs than dropdowns :).

vaidehi44 · 2022-07-22T18:26:38Z

Should I also modify the UI for the aggregated mainspace stats at the top, according to the design?

ragesoss · 2022-07-22T18:30:30Z

No, don't change the style of the overview stats.

…

On Fri, Jul 22, 2022, 11:26 AM Vaidehi Atpadkar ***@***.***> wrote: Should I also modify the UI for the aggregated mainspace stats at the top, according to the design? — Reply to this email directly, view it on GitHub <#5064 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGPEYYBGFQP54VNMKMGFZ3VVLRWRANCNFSM54I4E2RA> . You are receiving this because you commented.Message ID: ***@***.*** com>

vaidehi44 · 2022-07-22T18:35:43Z

It would be good to treat the titles in parallel, so that whatever parsing of the stats hash key is used, it works the same way for keys that include a namespace and those that don't. (ie, it should be able to handle www.wikidata.org, www.wikidata.org-namespace-120, en.wikibooks.org, and en.wikibooks.org-102.)

I didn't get this. Should the namespace titles and stats title be same? Or there shouldn't be any stats title?

ragesoss · 2022-07-22T18:36:21Z

maybe, but I don't think that's something that is a priority to display. although it will be easy to change later.

…

On Fri, Jul 22, 2022, 11:30 AM Vaidehi Atpadkar ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/assets/javascripts/components/common/OverviewStats/namespace_overview_stats.jsx <#5064 (comment)> : > + <OverviewStat + id="articles-edited" + className="stat-display__value" + stat = {statistics.edited_count} + statMsg={I18n.t('metrics.articles_edited')} + renderZero={true} + /> + <OverviewStat + id="total-edits" + className="stat-display__value" + stat = {statistics.revision_count} + statMsg={I18n.t('metrics.edit_count_description')} + renderZero={true} + /> + <OverviewStat + id="student-editors" Won't it give insight of how many students worked on particular tracked namespace? — Reply to this email directly, view it on GitHub <#5064 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGPEYZGQ42JKHCFBWPYVLLVVLSEHANCNFSM54I4E2RA> . You are receiving this because you commented.Message ID: <WikiEducationFoundation/WikiEduDashboard/pull/5064/review/1048264228@ github.com>

ragesoss · 2022-07-22T18:37:24Z

the domain, along with the namespace if present, should be a sufficient title in all cases.

…

On Fri, Jul 22, 2022, 11:35 AM Vaidehi Atpadkar ***@***.***> wrote: It would be good to treat the titles in parallel, so that whatever parsing of the stats hash key is used, it works the same way for keys that include a namespace and those that don't. (ie, it should be able to handle www.wikidata.org, www.wikidata.org-namespace-120, en.wikibooks.org, and en.wikibooks.org-102.) I didn't get this. Should the namespace titles and stats title be same? Or there shouldn't be any stats title? — Reply to this email directly, view it on GitHub <#5064 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGPEY5BVX74RAHWTEMOIKDVVLSYVANCNFSM54I4E2RA> . You are receiving this because you commented.Message ID: ***@***.*** com>

vaidehi44 · 2022-07-24T18:51:14Z

@ragesoss I have pushed most of the changes suggested except tabs UI. I feel that the tabs design which you have shared is too ordinary 😅. So, I tried out some stuff myself. Do you think, we can may be try something from these, or anything else?

ragesoss · 2022-07-25T15:54:29Z

I like the third one!

ragesoss · 2022-07-25T16:00:30Z

app/assets/javascripts/components/common/overview_stats_tabs.jsx

+ const title = statsList[currentTabId].statsTitle;
+ const data = statsList[currentTabId].statsData;
+ const content = (title === 'www.wikidata.org')
+ ? <WikidataOverviewStats statistics={data} isCourseOverview={true}/>


It might make sense here to implement another simple component (eg, OverviewStatsTab (singular)), so that this tabs component has a simple job: render all the tabs and render the content for the current tab. The new component would be in charge of which component to render for the current tab content.

Well, I have already created a component named OverviewStatsTab which renders a tab.
May be we can name it OverviewStatsContent or OverviewStatsData or anything like that. And, you are saying that the logic of identifying the content (whether it is wikidata or namespace stats) according to active tab, should be done in this component? Instead of having statsList, we should use this new component?

You're right, OverviewStatsTab would be a bad name for it, and the component you have now called that is appropriately named. OverviewStatsContent sounds good.

And yes, I think having this component identify wikidata vs namespace stats should be done in that new component.

@ragesoss, I have created the OverviewStatsContent component. Please check it along with OverviewStatsTabs to confirm if that's okay.

yes, it looks good to me.

ragesoss · 2022-07-25T16:02:06Z

app/assets/javascripts/components/common/wikidata_overview_stats.jsx

-const WikidataOverviewStats = ({ statistics }) => {
+const WikidataOverviewStats = ({ statistics, isCourseOverview }) => {
+ let containerClass = 'wikidata-stats-container';
+ let title = 'Wikidata stats';


It looks like this title isn't used in the the latest screenshots you shared?

Yes, it is used. In the last one. Here, the title isn't "Wikidata stats", but "www.wikidata.org". I thought that this would be in accordance with other namespace stats and their title. So, "Wikidata Stats" would be only shown for campaign overview stats. Is it okay?

ragesoss · 2022-07-25T16:09:10Z

spec/features/wikidata_course_spec.rb

@@ -23,7 +23,7 @@

 it 'shows Wikidata stats on the Home tab' do
 visit "/courses/#{course.slug}"
- expect(page).to have_content('Wikidata stats')
+ expect(page).to have_content('www.wikidata.org')


I'd like this PR to include a ruby feature spec for having multiple tabs and interacting with them. This file could be renamed detailed course stats or similar, and then the spec could be basically the same as this Wikidata one except with multiple tabs worth of data in the CoursesStats record.

The spec should test clicking through different tabs and seeing the different stats render.

…ikiEduDashboard into namespace-stats-ui

ragesoss · 2022-07-28T17:44:47Z

app/presenters/courses_presenter.rb

 course.course_stat.stats_hash['www.wikidata.org']
- end
+ }.compact


I think rubocop will complain about using braces for a multi-line block instead of do/end. You can still chain the do/end syntax with end.compact.

ohkk, thanks.

ragesoss · 2022-07-28T17:52:24Z

spec/features/detailed_course_stats_spec.rb

+ visit "/courses/#{course.slug}"
+ end
+
+ it 'shows tabs for all the course stats objects' do


I think it would be a little better to condense this into one test example, because only loading the page once will be a little faster, and all these tasks together represent a typical user flow: first load the page and see the expected tabs, then look at the default tab's content and confirm that at least one of the expected stats is being displayed (eg, 35 claimed created), then click to the next tab (ideally identifying it by the rendered label rather than the tab-1 id) and verify that the wikidata stats are gone and the new stats are there, then clicking the third tab and doing the same thing.

ragesoss · 2022-08-01T16:38:19Z

@vaidehi44 can you include before/after screenshots for the case of just Wikidata stats?

ragesoss · 2022-08-01T16:39:51Z

app/presenters/courses_presenter.rb

@@ -151,9 +151,9 @@ def trained_percent
 end

 def wikidata_stats
- stats ||= courses.joins(:course_stat).where.not(course_stats: nil).map { |course|
+ stats ||= courses.joins(:course_stat).where.not(course_stats: nil).filter_map do |course|


vaidehi44 · 2022-08-01T17:22:23Z

@vaidehi44 can you include before/after screenshots for the case of just Wikidata stats?

@ragesoss

Before

After

ragesoss · 2022-08-01T17:26:20Z

Okay! I think this is ready to merge. Anything you can think of that should be done before that?

vaidehi44 · 2022-08-01T17:31:16Z

@ragesoss, I think, one thing which needs to be done is to handle overflow of tabs if there are more number of tabs. Of course, this can be done later too as now in prod there won't be any tabs.

ragesoss · 2022-08-01T17:32:47Z

Good point. Do you have a plan for that? I'm fine with either merging this now and handling tab overflow in a separate PR, or including it in this one.

vaidehi44 · 2022-08-01T17:37:13Z

I have some ideas, but not a perfect plan. So, I think I can open a seperate a PR for this. We can may be try different things there.

ragesoss · 2022-10-11T07:11:11Z

I think either way is fine. Usually I prefer letting each component decide what to render, rather than passing components as props for others to render.

…

On Fri, Jul 22, 2022, 11:59 AM Vaidehi Atpadkar ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/assets/javascripts/components/common/overview_stats_tabs.jsx <#5064 (comment)> : > + let ns_title = ArticleUtils.NamespaceIdMapping[ns_id]; + if (typeof (ns_title) !== 'string') ns_title = ns_title[project]; + return `Stats for ${I18n.t(`namespace.${ns_title}`)} (${wiki})`; + }; + + const onTabChange = (e) => { + return setCurrentTabId(Number(e.currentTarget.id)); + }; + + const hasWikidataStats = () => { + if (statistics['www.wikidata.org']) return true; + return false; + }; + + const statsDataList = []; + // if there are wikidata overview stats, they should be displayed on first tab Also, I got this conceptual question. For each tab we have a different content rendered by different component. So, is it better to create this components, store and use them (like I am doing now), or is it better to initialize and render them instantaneously when a tab is selected? I think in our case, the compoents are not that heavy and so they can probably be stored before hand. — Reply to this email directly, view it on GitHub <#5064 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGPEY2QYD5Y433XF73FQOTVVLVPVANCNFSM54I4E2RA> . You are receiving this because you commented.Message ID: <WikiEducationFoundation/WikiEduDashboard/pull/5064/review/1048289980@ github.com>

vaidehi44 added 5 commits July 22, 2022 00:34

Add namespace-id mapping and make required modifications.

02da8da

Add components to display namespace overview stats in tabs.

8c237a3

Modify WikidataOverviewStats container class to adjust css.

a0bb519

If the wikidata stats component is used in course overview tabs, there the extra padding and border is not required. But, it is necessary if the component is displayed on campaign overview page.

Merge remote-tracking branch 'upstream/master' into namespace-stats-ui

0c81559

Some eslint fixes.

23821ba

ragesoss reviewed Jul 22, 2022

View reviewed changes

vaidehi44 added 5 commits July 23, 2022 10:13

Move stats title function to article utils.

918faf0

Refined some code.

8f2d6ca

Test for overview stats title util function.

9cc4b44

Minor changes

5670769

Lint fixes

7dd7f26

Fix ruby spec test.

f0e4e0b

ragesoss reviewed Jul 25, 2022

View reviewed changes

New stats content component.

c0b47bc

vaidehi44 added 8 commits July 27, 2022 01:11

New UI for tabs.

ae7d15d

Merge branch 'master' of https://github.com/WikiEducationFoundation/W…

f019e01

…ikiEduDashboard into namespace-stats-ui

Add method to get namespace id from title.

d8d7bf0

Write ruby test for tabbed course stats

49b696b

Change tab id

f7c9833

Update format method for all course stats

c170b1c

Merge remote-tracking branch 'upstream/master' into namespace-stats-ui

82d042f

Update renderZero prop for all namespace attributes

8aea637

ragesoss reviewed Jul 28, 2022

View reviewed changes

vaidehi44 added 3 commits July 30, 2022 01:03

Update feature test

1639e9f

Handle rendering of tabs if there's only one stats object

34b8c98

Fix some Lint errors

6f58d37

ragesoss reviewed Aug 1, 2022

View reviewed changes

ragesoss merged commit 1df0914 into WikiEducationFoundation:master Aug 1, 2022

enterstudio mentioned this pull request Nov 17, 2022

[Snyk] Fix for 1 vulnerabilities enterstudio/WikiEduDashboard#70

Open

enterstudio mentioned this pull request Nov 29, 2022

[Snyk] Fix for 1 vulnerabilities enterstudio/WikiEduDashboard#71

Open

Frontend changes for showing tabbed course overview stats #5064

Frontend changes for showing tabbed course overview stats #5064

Conversation

vaidehi44 commented Jul 21, 2022

What this PR does

Screenshots

ragesoss commented Jul 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragesoss commented Jul 22, 2022

vaidehi44 commented Jul 22, 2022

vaidehi44 commented Jul 22, 2022

ragesoss commented Jul 22, 2022 via email

vaidehi44 commented Jul 22, 2022

ragesoss commented Jul 22, 2022 via email

ragesoss commented Jul 22, 2022 via email

vaidehi44 commented Jul 24, 2022

ragesoss commented Jul 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragesoss commented Aug 1, 2022

Choose a reason for hiding this comment

vaidehi44 commented Aug 1, 2022 • edited Loading

Before

After

ragesoss commented Aug 1, 2022

vaidehi44 commented Aug 1, 2022

ragesoss commented Aug 1, 2022

vaidehi44 commented Aug 1, 2022

ragesoss commented Oct 11, 2022 via email

vaidehi44 commented Aug 1, 2022 •

edited

Loading