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

215 Logo lockup #252

Merged
merged 19 commits into from
Nov 21, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
215: Add attributes and modifier_class variables in twig comment
Co-Authored-By: yvonnetangsu <[email protected]>
Joe Knox and yvonnetangsu authored Nov 8, 2018
commit 24eaadda76a422e31cf8439817475d6e23725ebd
7 changes: 7 additions & 0 deletions core/templates/components/lockup/lockup.twig
Original file line number Diff line number Diff line change
@@ -19,12 +19,19 @@
* - .su-lockup__sitename-line5-uppercase - Uppercase, bold and smaller font for line 5
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, .su-lockup, .su-lockup__sitename-line1, .su-lockup__sitename-line2 and .su-lockup__sitename-line5 are not variants. Those classes are required in all cases in order for the component to render correctly. And indeed they're hardcoded in the template. I would argue for removing them from their respective list of "Available variants." Yvonne said she documented them that way in order to be consistent with existing doc, but we couldn't (quickly) find an example of that. @josephgknox & @sherakama, your thoughts on whether or not these required classes should be documented as "variants"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you mean, @JBCSU . My proposed tweak would be to list the "defaults" in their own section, and then the variants. @josephgknox and @sherakama , LMK what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I redid the comments in lockup.twig. LMK what you all think.

Choose a reason for hiding this comment

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

Thanks for bringing this up @JBCSU. I completely agree with you here. @yvonnetangsu thanks for addressing this already. My one (very small) comment would be to move the Default classes for the individual site name lines comment above the Available variant for the lockup comment so that class comments and variant comments are next to one another.

* Available variables:
* - attributes: For additional HTML attributes not already provided.
* - modifier_class: Additional css classes to change look and behaviour.
* - link: The URL that the lockup links to.
yvonnetangsu marked this conversation as resolved.
Show resolved Hide resolved
* - line1: Line 1 of the unit/site name text (to the right of the wordmark).
* - line2: Line 2 of the unit/site name text (to the right of the wordmark).
* - line5: Line 5 of the unit/site name text (at the bottom part of the lockup).
* - line1_modifier_class: Classes for line 1 font variants
* - line5_modifier_class: Classes for line 5 font variants
* - line1: Line 1 of the unit/site name text (to the right of the wordmark).
* - line2: Line 2 of the unit/site name text (to the right of the wordmark).
* - line5: Line 5 of the unit/site name text (at the bottom part of the lockup).
* - line1_modifier_class: Classes for line 1 font variants
* - line5_modifier_class: Classes for line 5 font variants
*/
#}
{% if line2 is not empty %}