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

feat/tup 637: Migrate c-card support list to core-styles #250

Merged
merged 15 commits into from
Nov 10, 2023

Conversation

R-Tomas-Gonzalez
Copy link
Collaborator

@R-Tomas-Gonzalez R-Tomas-Gonzalez commented Oct 26, 2023

Overview

Migrate c-card list support to core-styles

Outdated Notices

Warning
Should not be merged until the code is adapted to not conflict with Bootstrap .card in any Portal (both TUP and Core Portal).

Related

Changes

Importing c-card list support from tup-ui to core-styles

Testing

Verify Card styles here
Screenshot 2023-10-26 at 11 22 19 AM

UI

Before After
Screenshot 2023-10-26 at 10 38 08 AM Screenshot 2023-10-26 at 11 24 38 AM
Screenshot 2023-10-26 at 10 38 18 AM Screenshot 2023-10-26 at 11 24 47 AM

@R-Tomas-Gonzalez R-Tomas-Gonzalez added feature A new feature or replacement of existing feature minor A feature in backward-compatible manner labels Oct 26, 2023
@R-Tomas-Gonzalez
Copy link
Collaborator Author

Note:

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Functionally, works. Architecturally, off. But we'll get there.

Blame my haphazard use of Fractal naming (to understand, see "Fractal vs. BEM").

Please add the new styles to c-card.css, not c-card-list.css. You may continue to use _c-card--list.hbs for the name of the reusable demo template.1

Why?

These styles are for any list in a card (any card), not a list-specific "modifier"* of a card.

* A "modifier", as in the "M" of BEM. CSS Style Guide - Naming Conventions - BEM.

Fractal vs. BEM

In Fractal, to create a "variant" (a.k.a. BEM class modifier), one writes c-card--var-aka-mod. I used that same syntax for creating Handlebar templates to be included into a demo template i.e. _c-card--one-plain.hbs (the underscore hides the template from the demo navigation). Should I? Must I? I don't know. Feedback welcome.

Footnotes

  1. If feedback and discussion lead to new standard, we can update after these after this PR.

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

I'm taking a little more liberty on this one, to get it done sooner, so we can test in bulk this week on dev. But I let you review the change.

src/lib/_imports/components/c-card.css Outdated Show resolved Hide resolved
src/lib/_imports/components/c-card/_c-card--list.hbs Outdated Show resolved Hide resolved
src/lib/_imports/components/c-card/_c-card--list.hbs Outdated Show resolved Hide resolved
@@ -185,4 +185,20 @@
/* SEE: https://www.tacc.utexas.edu/education/k-12-students/high-school-camps/gencyber/ */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lines 184-187:
This has popped up twice (once in this branch, and second in another: 254)
I'm not sure how to handle this, and I'm not sure deleting it is the right choice. In 254, I did delete, but here, I just moved it to it's original spot. Let me know what you think @wesleyboar when you review this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Delete please.

  • The effect of the style is negligible.
  • The <h4> is demo'd by c-card--image-top for contact card.

@wesleyboar
Copy link
Member

wesleyboar commented Nov 9, 2023

I fixed a bug (92057e0). I also moved list to a card that has negative space (13ff9a0), so there are an even number of cards, thus less vertical scrolling.

wide, 4 per row medium, 3 per row narrow, 2 per row
wide 4 per row medium 3 per row narrow 2 per row

@wesleyboar wesleyboar self-requested a review November 9, 2023 23:38
@wesleyboar wesleyboar merged commit 58a3574 into main Nov 10, 2023
@wesleyboar wesleyboar deleted the feat/tup-637-c-card-support-list branch November 10, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature or replacement of existing feature minor A feature in backward-compatible manner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants