Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the RecommendedResourceCard to test KCard #4480

Merged

Conversation

AllanOXDi
Copy link
Member

@AllanOXDi AllanOXDi commented Mar 21, 2024

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
Screenshot 2024-03-27 at 15 01 14

layout: vertical thumbnailDisplay: none

Screenshot 2024-03-26 at 22 58 07

layout: vertical thumbnailDisplay: small

Screenshot 2024-03-26 at 22 58 45

layout: horizontal thumbnailDisplay: large

Screenshot 2024-03-26 at 22 59 18

Reviewer guidance

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
Copy link
Member

akolson commented Apr 15, 2024

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

@MisRob
Copy link
Member

MisRob commented Apr 16, 2024

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.

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

MisRob commented Apr 16, 2024

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
Copy link
Member

akolson commented Apr 16, 2024

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

@MisRob
Copy link
Member

MisRob commented Apr 16, 2024

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.

Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@AllanOXDi AllanOXDi mentioned this pull request Apr 22, 2024
24 tasks
@MisRob MisRob self-requested a review April 23, 2024 09:23
@MisRob
Copy link
Member

MisRob commented Apr 23, 2024

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?

@AllanOXDi AllanOXDi merged commit 04d07c1 into learningequality:unstable Apr 23, 2024
13 checks passed
</div>
</div>
</div>
</BaseCard>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Updating it as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1,404 @@
<template>

<span :style="rootContainerStyles">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants