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

Re-organize KCard styles to simplify and improve content tolerance #838

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Nov 21, 2024

Description

  • Fixes the thumbnail overflow problems in horizontal layout with small thumbnail
  • Re-organizes KCard styles to improve content tolerance and simplify styles
  • Introduces cards QA page available on /qa/cards URL
    • So that we don't need to copy-paste cards to the playground every time when updating card
    • The page can be later removed in favor of automated visual testing

This is rather a larger refactor of styles organization. To get a high-level sense, I recommend you preview the Figma illustration in the Implementation notes.

Before After
before-1 after-1
before-2 after-2

Changelog

  • Description: Re-organizes KCard styles to improve content tolerance and simplify styles, and fixes the thumbnail overflow problems in horizontal layout with small thumbnail
  • Products impact: bugfix
  • Addresses: Thumbnail overflow problems in horizontal layout with small thumbnail experienced in Kolibri and Studio
  • Components: KCard
  • Breaking: no
  • Impacts a11y: no
  • Guidance: -

Steps to test

Implementation notes

At a high level, how did you implement this?

Figma to the rescue (source).

KCard structure

Does this introduce any tech-debt items?

No. Quite opposite - allows for removal of complex or fragile logic. And to my surprise simplified many other places.

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)?

@MisRob MisRob force-pushed the content-tolerance-card branch from a0fb5ca to 81d7a27 Compare November 21, 2024 17:06
Fixes thumbnail overflow problems in some layouts
and makes styles more robust and better organized
overall.
@MisRob MisRob force-pushed the content-tolerance-card branch from 81d7a27 to f19114a Compare November 21, 2024 18:15
@MisRob MisRob marked this pull request as ready for review November 21, 2024 18:19
@MisRob
Copy link
Member Author

MisRob commented Nov 21, 2024

@akolson @AlexVelezLl @AllanOXDi It may more sense to review KCars styles as a whole rather than using the diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO @MisRob: move to docs/pages/playground/cards.vue as per @AlexVelezLl feedback

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.

Thanks you @MisRob!! I just left a couple of small suggestions/considerations we can discuss about. Let me know what you think :)

margin-bottom: calc(#{$spacer} / 2);
}
.above-title {
padding-bottom: calc(#{$spacer} / 2);
Copy link
Member

Choose a reason for hiding this comment

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

As .around-title is a flex. We can add just one line: gap: calc(#{$spacer} / 2) to it so we can have this space to evenly separate the title, above title, and below title. Without having to explicity set these padding bottom and top.

@@ -698,104 +704,92 @@
}
}

.horizontal-with-large-thumbnail {
$thumbnail-width: 40%;
.horizontal-with-small-thumbnail {
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion: What if we play with the flex-direction and gap attributes to get this working without too much specific customization. For example instead of having this:

  .horizontal-with-small-thumbnail {
    .upper-card-area {
      display: flex;
      flex-direction: row;
      align-items: flex-start;
    }

    .thumbnail {
      width: 30%;
      margin-top: $spacer;
      margin-bottom: $spacer;
    }

    .around-title {
      width: 70%;
    }

    &.thumbnail-align-left {
      .thumbnail {
        order: 1;
        margin-left: $spacer;
      }

      .around-title {
        order: 2;
      }
    }

    &.thumbnail-align-right {
      .thumbnail {
        order: 2;
        margin-right: $spacer;
      }

      .around-title {
        order: 1;
      }
    }
  }

This would also work as expeced:

  .horizontal-with-small-thumbnail {
    .upper-card-area {
      display: flex;
      flex-direction: row;
      align-items: flex-start;
      padding: $spacer;
      gap: $spacer;
    }
    .thumbnail {
      width: 30%;
    }
    .around-title {
      width: 70%;
      padding: 0;\
    }
    &.thumbnail-align-left {
      .upper-card-area {
        flex-direction: row-reverse;
      }
    }
  }

With this the proposal is:

  • Manage left and right alignments with row and row-reverse flex directions.
    • With this, we should not need a thumbnail-align-right class. Just a thumbnail-align-left class to inidicate when to set the row-reverse direction
  • Let upper-card-area manage the upper card spacing both for external and internal gap. And avoid relying on around-title to set its padding.
    • With this there is no need to set margins to the thumbnail.

    • Another side small winning with this? Better calculations for 30%/70% widths. Before, as around-title used to set its internal padding, this padding also served as the gap between the thumbnail and the around title. This made that this gap space were not taking the same proportions from both elements, but just from one. I am not sure if I am explaining myself well, as we are working with 30/70 this is not that obvious that this is "a problem" but lets assume we would need a 50/50 layout, this would make more obious that the thumbnail is being rendered a bit bigger, because it is not "paying" space to set the gap between it and the title. Visual example of this: With the current solution if we had a 50/50 layout it would look like this, where it seems that the thumbnail is taking more than the half of the space:

      image

      But with the new solution this would look more balanced:

      image

      This is a very small consideration, and its obvious that is not that noticeable with the 30/70 layout, but its worth mentioning it :).

Here is a before/after comparison of rendered cards, where almost everything looks the same. The "after" card thumbnail is a little bit smaller because of the previous explanation.

Before After
image image

A downside with this, is that we should reconsider the implications of extending this to .horizontal-with-large-thumbnail

@MisRob
Copy link
Member Author

MisRob commented Dec 2, 2024

Hi @AlexVelezLl, thank you, I appreciate your ideas and think they could indeed simplify styles even more which would be great. I am happy to experiment with it. Would you mind if I did this in a follow-up to have a bit more room to play around with it? The current thumbnail bug is quite significant, so it would be good to release the fix as soon as possible if there's nothing blocking.

@AlexVelezLl
Copy link
Member

Yes @MisRob! That sounds good. We can then have a broader discussions about further implications there :).

@MisRob
Copy link
Member Author

MisRob commented Dec 4, 2024

Thank you @AlexVelezLl. Issue with myself assigned here #850.

Something else needed here, or can we merge?

@AlexVelezLl
Copy link
Member

Hi @MisRob! I think the only thing that is pending is the rename of the qa folder. Apart from that everything looks good to me :3.

@MisRob
Copy link
Member Author

MisRob commented Dec 4, 2024

Ah I forgot about that. Thanks.

@MisRob
Copy link
Member Author

MisRob commented Dec 4, 2024

Done @AlexVelezLl

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.

Thanks @MisRob! Looks good to me! :)

@MisRob MisRob merged commit 56c3cdc into learningequality:develop Dec 4, 2024
10 checks passed
learning-equality-bot bot pushed a commit that referenced this pull request Dec 4, 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.

2 participants