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

fixes card display without a thumbnail #752

Merged
merged 10 commits into from
Sep 11, 2024

Conversation

AllanOXDi
Copy link
Member

@AllanOXDi AllanOXDi commented Aug 25, 2024

Issue addressed

#699

Changelog

  • Description: UpdateKCard to complete vertical/horizontal layouts with no thumbnail
  • Products impact: Card updates
  • Addresses: fixes card display without a thumbnail #752
  • Components: KCard
  • Breaking: N0
  • Impacts a11y: No
  • Guidance:

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

After review

  • The changelog item has been pasted to the CHANGELOG.md

Comments

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Thank you @AllanOXDi! I left a couple of questions as I saw some padding issues in some examples.

lib/KCard/index.vue Show resolved Hide resolved
lib/KCard/index.vue Outdated Show resolved Hide resolved
lib/KCard/index.vue Outdated Show resolved Hide resolved
docs/pages/playground.vue Outdated Show resolved Hide resolved
docs/pages/playground.vue Outdated Show resolved Hide resolved
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.

@AllanOXDi We can't condition the presence of the thumbnail area by the presence of the thumbnail source and other factors you used to determine whether the area should be displayed. We would loose functionality that shows thumbnail placeholders when a thumbnail source is not available.

The only determining factor is thumbnailDisplay prop. Basically the idea is quite simple - if the thumbnail area is configured to be present via this prop, is shows no matter whether the thumbnail source is provided. If the thumbnail source is not provided in this case, the thumbnail area shows as a placeholder.

So let's just:

<div
  v-if="thumbnailDisplay !== ThumbnailDisplays.NONE"
  class="thumbnail"
>

All other conditions need to be reverted.

This is important because we quite frequently display placeholders in Kolibri. One way to observe this would be on playground - try to see what happens when you don't provide thumbnailSrc but still want to display the thumbnail area via thumbnailDisplay. Also I'd recommend you try to use #thumbnailPlaceholder slot.

Again, visual tests will help us greatly one day :) Until then, with every change we will need to manually preview the card in all related modes.

lib/KCard/index.vue Outdated Show resolved Hide resolved
lib/KCard/index.vue Outdated Show resolved Hide resolved
lib/KCard/index.vue Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Before merging, would you remove CHANGELOG.md changes from this PR completely and filled in the template in the pull request description instead? The new action will then paste it to CHANGELOG.md from there.

lib/KCard/index.vue Outdated Show resolved Hide resolved
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.

Thank you for the updates @AllanOXDi, all good!

Just two cleanup notes that would be good to push before merging and then feel free to merge.

@AllanOXDi AllanOXDi force-pushed the Card-fix branch 2 times, most recently from f9564da to 4ab4bae Compare September 5, 2024 08:02
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.

I will put changes request here temporarily so we don't merge accidentally before changelog.md cleanup finished.

@MisRob MisRob self-requested a review September 11, 2024 15:07
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.

Thank you

@AllanOXDi AllanOXDi merged commit 2b03efb into learningequality:develop Sep 11, 2024
8 checks passed
learning-equality-bot bot pushed a commit that referenced this pull request Sep 11, 2024
KshitijThareja pushed a commit to KshitijThareja/kolibri-design-system that referenced this pull request Dec 17, 2024
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