-
Notifications
You must be signed in to change notification settings - Fork 687
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
Card fixes #12374
Card fixes #12374
Conversation
Build Artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am kind of confused by the names.
@@ -45,7 +45,7 @@ | |||
:title="content.title" | |||
:thumbnail="content.thumbnail" | |||
:description="content.description" | |||
:kind="content.kind" | |||
:kind="content.is_leaf ? content.learning_activities : 'folder'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change the prop name here to indicate that it is no longer accepting a content kind?
v-if="!windowIsSmall" | ||
class="thumbnail" | ||
:thumbnail="thumbnail" | ||
:kind="kind" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that no updates have been made to the CardThumbnail component, but we are now passing a learning activity array to this, rather than a content kind, I haven't checked, but I expect this to end poorly.
<CoachContentLabel | ||
class="coach-content-label" | ||
:value="numCoachContents" | ||
:isTopic="isTopic" | ||
/> | ||
</div> | ||
<slot name="notice"></slot> | ||
<LearningActivityChip | ||
v-if="isLeaf" | ||
:kind="kind" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a kind or a learning_activity? I know the component was already like this, but this naming all seems very confusing.
Hi @marcellamaki - looks great in both desktop and mobile view, couldn't spot any issues caused by the changes made here. I'm gonna mention that I noticed the following issue with overlapping learning activity chips, which is preexisting and I'm not sure whether is not caused by anything on Studio's end: Overlapping.chips.mp4You can replicate it by going to the following resource Hoja de Reflexión para Estudiantes - Acertijos Wiixii |
…y. Not strictly needed yet, but will be soon for card refactoring work
…nd present a UI that's closer to 0.18 refactor
… and Icon, and remove the files from Learn in preference of kolibri-common
….kind and content.learning_activities. there be dragons/tech debt there
}, | ||
isLeaf: { | ||
type: Boolean, | ||
content: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a tech debt cleanup, we should define a content node spec using the core validator stuff, so that we can just reuse that every time we want content/contentnode as a prop. I would be happy to be assigned to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confident now that even if the LearningActivityChip component exposes a very misleading prop name, it and the CardThumbnail component are both being given the correct values.
Summary
Makes some mostly-cosmetic card layout and styling adjustments. Does a teensy refactor to put the learning activity chips and icon mapping into kolibri-common, which will be used in 0.18 as part of the card and refactoring work.
References
Fixes #12280
Related slack conversation
Reviewer guidance
Testing updates:
content cards (both channel and resources) used in coach resources management
Learn:
I refactored the "LearningActivityChip" to
kolibri-common
so the places where it is used in learn should be regression tested.It's present in the resource's information side panel accessed in
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)