Update tree view to include new metadata

[marcellamaki]

Summary

Description of the change(s) you made

Fixed #3206

Adds titles and metadata in the Resource Panel for Studio.
Does not add or update the Learning Activity Label or icons (follow up PR forthcoming)

Manual verification steps performed

1. Added test data with ids from le-utils to test the parsing of ids and strings in each of the new categories

Screenshots (if applicable)

---

Reviewer guidance

How can a reviewer test these changes?

Are there any risky areas that deserve extra testing?

References

Comments

---

Contributor's Checklist

PR process:

• If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
• If this includes an internal dependency change, a link to the diff is provided
• The docs label has been added if this introduces a change that needs to be updated in the user docs?
• If any Python requirements have changed, the updated requirements.txt files also included in this PR
• Opportunities for using Google Analytics here are noted
• Migrations are safe for a large db

Studio-specifc:

• All user-facing strings are translated properly
• The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
• All UI components are LTR and RTL compliant
• Views are organized into pages, components, and layouts directories as described in the docs
• Users' storage used is recalculated properly on any changes to main tree files
• If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

• Code is clean and well-commented
• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical user journeys are covered by Gherkin stories
• Any new interactions have been added to the QA Sheet
• Critical and brittle code paths are covered by unit tests

---

Reviewer's Checklist

This section is for reviewers to fill out.

• Automated test coverage is satisfactory
• PR is fully functional
• PR has been tested for accessibility regressions
• External dependency files were updated if necessary (yarn and pip)
• Documentation is updated
• Contributor is in AUTHORS.md

[marcellamaki]

@rtibbles @bjester -- I am going to make the PR description more robust for Jessica's review with screenshots to ensure I am displaying all scenarios correctly, but I would be happy to have some eyes on the code to see if I have actually done what I think I have done :) So, if you have time to take a look, that would be great. Thank you!

[rtibbles]

A couple of updates needed.

[marcellamaki]

Updated views

This PR has been updated to include the card and resource panel display of metadata on default, compact, and comfortable views with the new metadata icons.

At this point, the only outstanding piece will be to add completion and duration display as a follow up issue

Figma, for reference

Note: after discussion in KDS re: existing icon colors and accessibility concerns - we opted to move forward with the grey chip with colored icon.

Compact view, with resource panel open (part 1)
Compact view, with resource panel open (part 2): previous step has multiple learning activities
Compact view, with resource panel open: current item has multiple learning activities
Comfortable view (with description)
Comfortable view (w/o description)
Default view

[MisRob]

Thank you, @marcellamaki . Below you can find notes that I think would be good to address (some may need only a clarification) before we merge. I’ll also post some comments in the code and mark those that are not blocking appropriately.

1. Possible visual regressions in other features

Updates in this PR affect other features, not only the channel tree navigation. One example would be the “Import from channels” feature:

Before	After

Before we merge this, I’d appreciate clarification on whether this was intended because from the user's point of view, the new version seems to be confusing to me since now they won’t be able to understand resources presented very well when importing from other channels.

I haven’t checked other features that may be affected by the update, this is only one example. In general, I’d suggest uploading before/after changes for every UI update. I know it can be lots of work and you already made a great effort and uploaded tons of screenshots, but don’t know about any better way how to prevent regressions or communicate that there were changes in places beyond the new feature implemented. As for the reviewer role, it’s hard to find all the places to check, and even when one can derive something from code, it’s hard to recognize whether changes were intended or not.

2. @formatjs/intl

Is there any advantage of installing the whole library when compared to using polyfills together with capability detection? Generally speaking, I’d say it’s better to include only what’s needed so that the app won’t grow and that native APIs will be used whenever possible. I can see that @rtibbles made some suggestions about polyfills earlier, any thoughts?

3. Lack of tests for new components or updates to existing ones

One of the examples would be the new ContentNodeLearningActivityIcon which contains lots of conditional logic which causes that (at least to me) it takes a while to understand all scenarios that could happen even when it’s a relatively simple component. There are also updates to existing components like ResourcePanel and Thumbnail. There’s no need to have a complex test suite, just a couple of main scenarios would be helpful.

[MisRob]

Not blocking, this can stay as is. Just an idea for rows 9-15 if you'd find it helpful. Maybe having another component, TopicIcon, separated from ContentNodeLearningActivityIcon would help with understanding and using ContentNodeLearningActivityIcon.

[marcellamaki]

Hi @MisRob - thank you for your comprehensive review!
Following this and the discussion in slack, I've added back in the original thumbnail and created a new temporary component as we transition to the new thumbnail component to prevent regressions. I also implemented the check for the polyfill, and added a comment re: $formatList. In our i18n setup, we are using other formatting options such as $formatNumber and $formatDate. However, due to a bug in formatjs, $formatList was excluded, which is why we are having to do this polyfill in general. I added a comment to explain, and a link to the upgraded version of (it was just fixed last week while I was working on this! but is a huge version jump).
I also added tests for ContentNodeLearningActivityIcon, which also prompted me to do some simplifying and refactoring in that file. 😄

[MisRob]

Thank you, @marcellamaki. Regarding polyfills, thank you for the comment, I wasn't aware of the relationship to vue-intl and the bug. So now I'm wondering whether my first comment about not using the whole @formatjs/intl library was relevant because I assume we somehow use it under the hood, anyways? I'm sorry for the complications if that's the case. I'll leave the final decision on you and @rtibbles and try to follow to understand this better. Also, if the whole polyfill thing shows up to be complicated, I think it'd be fine not to use Intl in this PR and do it as a follow-up issue.

---

If we want to keep using just polyfills instead of bringing in the whole library as I suggested earlier and is implemented in your latest changes, some last touches would be needed:

• Intl.ListFormat polyfill has two dependencies (Intl.getCanonicalLocales or polyfill, Intl.Locale or polyfill). If I can understand their documentation (here and here), we need to load them too.
• I can see two polyfill functions, one in the ResourcePanel and another in shared/i18n/index.js. One of them needs to be removed. I think that keeping the latter makes more sense when compared to loading the polyfill in a component. Just note that the polyfill function needs to be called in that file.
• Some cleanup of the previous implementation is needed - uninstallation of @formatjs/intl and not using createIntl (this should help with failing tests).

To put this all together, I think that we only need to have @formatjs/intl-listformat , @formatjs/intl-getcanonicallocales,@formatjs/intl-locale installed and the polyfill function could look something like this (haven’t tried, just pulled pieces from documentation):

import {shouldPolyfill as shoudPolyfillCanonicalLocales } from '@formatjs/intl-getcanonicallocales/should-polyfill'
import {shouldPolyfill as shouldPolyfillLocale } from '@formatjs/intl-locale/should-polyfill'
import {shouldPolyfill as shouldPolyfillListFormat} from '@formatjs/intl-listformat/should-polyfill'

async function polyfill(locale) {
  if (shoudPolyfillCanonicalLocales()) {
    await import('@formatjs/intl-getcanonicallocales/polyfill')
  }
	if (shouldPolyfillLocale()) {
    await import('@formatjs/intl-locale/polyfill')
  }
	const unsupportedLocale = shouldPolyfillListFormat(locale)
  if (!unsupportedLocale) {
    return
  }
  await import('@formatjs/intl-listformat/polyfill-force')
  await import(`@formatjs/intl-listformat/locale-data/${unsupportedLocale}`)
}

[MisRob]

New tests are looking good to me, thank you. I also skimmed briefly through ImageOnlyThumbnail and haven't noticed anything outstanding. I didn't study styles much though as I can see most of it comes from the original thumbnail component that have worked well for some time I think.

[marcellamaki]

@MisRob, following the discussion in the planning meeting, I've updated just to remove the polyfill, and use the default Intl.ListFormat (since it aligns with the browsers we support). I hope after that final commit is reviewed, this is ready, but please let me know if there is anything else you see 😄

[MisRob]

Thank you, @marcellamaki. The latest updates are looking good to me (haven't tested again manually). Regarding code, the only thing - could you revert changes of package.json (and therefore of yarn.lock as well)? We don't need @formatjs/intl in dependencies now,

[MisRob]

Thanks for your patience, and sorry for the complications about polyfills, I misunderstood our needs and dependencies.

[rtibbles]

I misunderstood our needs and dependencies.

I think the lack of clarity wasn't helped by me either - plenty of blame to go around!

Comments addressed.

[pcenov]

Looks good to me, have reported one related issue separately: #3664

[marcellamaki]

Thanks @pcenov -- I will assign that issue to myself to fix the remaining problems!