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

KCardGrid: Part 1 - Prepare KCard + few bugfixes and a visual spacing update #774

Merged
merged 25 commits into from
Sep 13, 2024

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Sep 10, 2024

Note that I worked from @AllanOXDi's #752 branch which hasn't been merged yet so the diff is visible in this PR. Please ignore @AllanOXDi's commits here. I will rebase as soon as #752 is merged.

Description

This is a first part of work that was proposed and discussed in #748.

  • Merges KCard and BaseCard to unblock several issues for the upcoming features regarding the addition of checkboxes and KCardGrid
  • Prepares KCard structure and style for the aforementioned features
  • Contains few bug fixes and docstrings improvements

See commit messages for details.

References

Closes #773

Changelog

  • Description: Merges KCard and BaseCard

  • Products impact: none

  • Addresses: Unblocks several issues for the upcoming features regarding the addition of checkboxes and KCardGrid

  • Components: KCard

  • Breaking: no

  • Impacts a11y: -

  • Guidance: -

  • Description: Updates KCard internal structure and style

  • Products impact: This introduces temporary regressions in KCard related to removing its control of its height that will instead be controlled by KCardGrid. However, KCard is required to be always used within KCardGrid anyways so ultimately this will be of no real impact. Will be completed by KCardGrid soon.

  • Addresses: Prepares KCard for the upcoming features regarding the addition of checkboxes and KCardGrid

  • Components: KCard

  • Breaking: no

  • Impacts a11y: -

  • Guidance: -

  • Description: Fixes the thumbnail overflowing in the horizontal layout with small thumbnail and aligns this layout more closely to the designs.

  • Products impact: bugfix

  • Addresses: Make the KCard thumbnail handle large Card width #773

  • Components: KCard

  • Breaking: no

  • Impacts a11y: -

  • Guidance: -

  • Description: Fix click.stop not working on interactive elements rendered within the card via its slots.

  • Products impact: bugfix

  • Addresses: -

  • Components: KCard

  • Breaking: no

  • Impacts a11y: -

  • Guidance: -

  • Description: Aligns padding to the designs

  • Products impact: Visual update

  • Addresses: -

  • Components: KCard

  • Breaking: no

  • Impacts a11y: -

  • Guidance: -

Steps to test

  • As this move was already discussed and agreed on, in this PR we would want to focus on catching any unexpected major regressions, particularly in the markup structure or layout. See the playground page.

  • Note that this introduces temporary regressions in KCard related to removing its control of its height (elements overflowing the card area, cards not having consistent height) that will instead be controlled by KCardGrid. This is deliberate step that will be completed by KCardGrid soon. It shouldn't cause any issues since KCard shouldn't be used without KCardGrid and consumers are instructed to link to [BETA] Card grid and checkboxes #748 before all that work is transitioned to develop.

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

AllanOXDi and others added 18 commits September 5, 2024 10:50
This unblocks several issues related to the current
internal architecture - primarily gives us more
flexibility needed to achieve simple public API
for card, card grid, and card checkboxes.
to better reflect its purpose.
rather than outer <li>.

This prepares ground for elements
that need to be rendered within <li>
but outside the card area, such as
checkboxes.
Previously, heading styles needed to be dynamic
so they could be passed down to BaseCard.
BaseCard is removed now so we can use much
simpler approach that also allows for removal
of the two copies of same styles in JavaScript
and CSS parts.
- remove duplicate information
- improve generated documentation display
- improve clarity
- re-order props to related groups
- try to be conscise
Prepares ground for card grid work
that will be responsible for controlling
card dimensions.
`relative` is already applied
via `card-area` class.
in the horizontal layout with small thumbnail.

Fixes the thumbnail overflowing
and aligns this layout more closely
to the designs.
@MisRob MisRob marked this pull request as ready for review September 10, 2024 07:55
@MisRob MisRob requested review from AlexVelezLl and AllanOXDi and removed request for AlexVelezLl and AllanOXDi September 10, 2024 09:18
@AllanOXDi
Copy link
Member

Hi @MisRob thanks for the wonderful work here. I am leaving few questions or comments here.

Copy link
Member

@AllanOXDi AllanOXDi left a comment

Choose a reason for hiding this comment

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

I am not sure if we could keep cards to uniform sizes of the card in this PR regardless whether the slots/ thumbnail is provided. I feel like something changed along the way

Screenshot 2024-09-11 at 17 27 52 Screenshot 2024-09-11 at 17 32 07

@MisRob
Copy link
Member Author

MisRob commented Sep 11, 2024

Hi @AllanOXDi, ah yes that's right. Thank you. All these issues are addressed in the PR that comes after this one. All of them are the temporary regressions I meant in the "Steps to test" - I will be more specific next time. This is caused by removing the height control from KCard and transitioning it to KCardGrid that will control the row height :) You can preview how it will eventually look like in #748 playground - I'm just breaking it down to smaller PRs.

@MisRob
Copy link
Member Author

MisRob commented Sep 11, 2024

I am adding

@MisRob MisRob changed the title KCardGrid: Part 1 - Prepare KCard KCardGrid: Part 1 - Prepare KCard + few bugfixes and a visual spacing update Sep 11, 2024
'mouseup' was replaced by 'click' event
in the previous work so tests
need to reflect it
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.

This looks good to me! I think the merge of BaseCard and KCard has had a good and cleaner result. Just left 2 super minor things in docs wording now that you already did some changes there 😅. Thank you!

lib/KCard/index.vue Outdated Show resolved Hide resolved
lib/KCard/index.vue Outdated Show resolved Hide resolved
lib/KCard/index.vue Show resolved Hide resolved
@MisRob
Copy link
Member Author

MisRob commented Sep 13, 2024

. Just left 2 super minor things in docs wording now that you already did some changes there 😅.

No worries @AlexVelezLl, I welcome that. Will look into it :) Thank you.

@MisRob MisRob merged commit 211b8e6 into learningequality:develop Sep 13, 2024
8 checks passed
learning-equality-bot bot pushed a commit that referenced this pull request Sep 13, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the KCard thumbnail handle large Card width
3 participants