Implement the RecommendedResourceCard to test KCard

[AllanOXDi]

Summary

• This PR add new RecommendedResourceCard component to studio.
  • Wraps around the KCard component
  • Implements KIcon for rendering icons and KIconButton for clickable icons

Screenshots (if applicable)

Layout horizontal thumbnailDisplay small

layout: vertical thumbnailDisplay: none

layout: vertical thumbnailDisplay: small

layout: horizontal thumbnailDisplay: large

Reviewer guidance

• Pull this PR on KDS and link it to your local Kolibri for the new icon to show Add thumb up and down icon kolibri-design-system#603
• You can import RecommendedResourceCard to where you want to use it and start using it while testing for these specifications KCard Technical Specification kolibri-design-system#528 #4450
  #530
• Run yarn run test contentcuration/contentcuration/frontend/channelList/views/KCard/__test__/KCard.spec.js to test the new KCard component. Do the test make sense? are all test cases captured?

References

#4450
#530

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

[akolson]

Also, noting that there are a few failing tests that we will need to handle

[MisRob]

Also, noting that there are a few failing tests that we will need to handle

Thanks @akolson, I think we will have a look at the test suite as soon as we move code into KDS if that's fine since we were planning to co-hack with @AllanOXDi on it and it's nothing blocking. That said, @AllanOXDi, this will mean that we will need to remove the test suite from this PR before merging, because all tests on unstable need to pass.

[akolson]

Hi @AllanOXDi @MisRob! The pr LGTM, thanks. Regarding QA testing of the component, are we moving it to KDS so testing can be done there, or still using the reference in studio?

[MisRob]

If the current logic is sufficient basis for future recommended resources work @akolson, we would do more detailed QA in KDS environment. In that case, before merging this PR, we would remove or comment out some code and later installed it from KDS.

[akolson]

Yes, this is good to go from a Recommended Resources perspective. Thanks @MisRob. Can we removal/commenting out of code?

[MisRob]

Thanks for confirming @akolson. Yes, we will walk through it with @AllanOXDi before merging (probably next Monday) and move code and log remaining tasks to KDS while removing/commenting out those parts from here.

[MisRob]

"requesting changes" only to block this from merging before we wrap up KDS transition

[MisRob]

Great, thanks @AllanOXDi.

Could you please open a Studio issue with a list of files that we're now temporarily merging and that need to be removed after we have KCard ready and released in the KDS repo?

[bjester]

No issue, but I just wanted to note the significance of semantic elements, for future consideration. Consider this structure (simplified for example):

<section>
  <header>
    <!-- titles and other header slots -->
  </header>
  <aside>
    <!-- thumbnail -->
  </aside>
  <div>
    <!-- main content -->
  </div>
  <footer>
    <!-- footer buttons etc. -->
  </footer>
</section>

[AllanOXDi]

Thanks! Updating it as well.

[MisRob]

Thank you @bjester. I too thought this would be good so I asked @AllanOXDi to incorporate it, but I think neither of us realized what @radinamatic mentions in this comment learningequality/kolibri-design-system#625 (comment), so I believe this is being reverted.

[bjester]

Perhaps someone else worked on this, and it was simply vendored here. Anyways, the many <span> tags here aren't appropriate because the purpose of this is to display it as 'block-level content'. Using <div> will simplify the styles because <span> is for 'inline-level'

[AllanOXDi]

Thanks @bjester for noting this - I have addressed it on KDS

[AllanOXDi]

Edit- this was not dressed @MisRob had a reason why

[MisRob]

I can understand that using div would simplify styles. Using spans was intentional approach. If KImg was implemented as div and then used within span, which is a valid use-case, then the resulting markup would be <span><div>...</div></span>. Generally, placing block level elements within inline level elements is discouraged if not disallowed (I don't recall the source anymore, but technically it may actually be allowed in HTML5 though - still I wouldn't consider it very intuitive so why to do so).

More high-level, it is a part of an approach that separates HTML semantics from styling, which is a pattern I lean towards generally as often it helps with accessibility. It is true that in some cases it can add further work with styling.

No problem to discuss this further together @bjester, I just said @AllanOXDi that he doesn't need to be dealing with it in the scope of the card work.

[bjester]

Yeah so instead of manipulating the styling, you can also manipulate the tag: <component :is="...">

This can be helpful for semantic structures, because lets say you code a generic component as a <section> but then you realize there's a case for using it as an <article> or an <aside>. All of those adhere to the same semantic structural scenarios in which to use <header> and <footer> as descendants.

[MisRob]

Yes that sounds like a useful technique for cases you need to switch element based on context