-
Notifications
You must be signed in to change notification settings - Fork 19
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
New component for content types #589
New component for content types #589
Conversation
Waiting on results of this PR #571 in regards to adding the |
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.
Minor thing and question.
@Snugug can you look at the scss please
@@ -0,0 +1,30 @@ | |||
.card { |
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 .card
generic on purpose? Would this styling be only for the content card, or any card?
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 think it could be for any card, although I'm not sure what those cards would be at this point. @Snugug can comment if this is the right approach or if it should be more specific.
@@ -63,6 +63,20 @@ module.exports = { | |||
], | |||
], | |||
}, | |||
contentData: { | |||
name: 'Careers', | |||
description: 'A section realted to joining Watson and what the culture of Watson is about', |
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.
Just for clarity sake, can you make this description sound more like it's describing a content type instead of a section of a website? Slight wording change should prol be enough
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.
Hmm... I pulled that description directly from the Careers content type. But for clarity, I can update here.
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.
Approve PR.
@Snugug all yours
@@ -0,0 +1,30 @@ | |||
.card { |
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.
If the file name is content-card
this component should be .content-card
, not .card
. Card is a little too generic, maybe we want a more descriptive name, like basic-card
or minimal-card
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.
ok. I like basic-card
so let's go with that.
padding: setting-get('rhythm') / .75; | ||
|
||
&--link { | ||
@include link(setting-get('active link color'), setting-get('active link color'), setting-get('secondary link color'), setting-get('secondary link color')); |
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.
This is identical to .base--link
. Instead of duplicating this, add a base-link
mixin to https://github.com/punchcard-cms/punchcard/blob/master/src/sass/partials/global/_mixins.scss with these defaults (or, alternatively, update the link
mixin with these as the default arguments) and make sure that .base--link
calls the mixin
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.
Just realized that I'm not actually using this class for this component, but I will still make the base-link
mixin change.
} | ||
|
||
&--button { | ||
@include button(setting-get('primary accent color'), setting-get('primary accent color'), setting-get('primary color'), setting-get('primary accent color'), setting-get('primary color'), setting-get('primary color'), setting-get('primary color')); |
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.
Same comment as link above
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.
ok, got it
@@ -0,0 +1,35 @@ | |||
.card { |
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.
- Layouts begin with an underscore, so
._card
- This, again, needs a more descriptive component name, and the component name and the file name need to match
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.
ok
|
||
|
||
@include breakpoint(setting-get('two grid cards')) { | ||
margin-right: 4%; |
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.
Where does this 4% come from? That's not in line with any of our rhythm anywhere. We've pretty consistently been doing spacing using rem
and fixed sizes. Why the sudden change? And why this percentage?
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.
No sudden change, I still new to the base level styling that you guys have done in the past. No worries, I will adjust to make it consistent.
|
||
@include breakpoint(setting-get('two grid cards')) { | ||
margin-right: 4%; | ||
width: 48%; |
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.
Where does this 48% come from? Is this suppose to be 50% with a 2% margin?. Width should be 50% and there should be padding in the inside at a fixed width for the gutter
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.
Updated to padding
instead of margin
and mirrored the rhythm
to what we are already using on the form--field-container
. With that said, 48% is used since we have a border and need there to be gutter between the cards.
|
||
@include breakpoint(setting-get('fixed container')) { | ||
margin-right: 4%; | ||
width: 22%; |
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.
Again, as above, I'm assuming this is… 25%? With a 3% margin on each side? I'm not sure that math actually works out given the numbers here, but either way, previous comment
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.
updated with same changes as above
@@ -0,0 +1,12 @@ | |||
{% macro card(data) %} | |||
<article class="card" itemscope itemtype="http://schema.org/Product"> |
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.
The items in here don't always semantically contain a product. In fact, the sample data you have here specifically isn't a product. This doesn't necessarily need any more specific type of schema other than thing
if we can't guarantee the 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.
ok, updated to thing
<section class="_style-guide--group"> | ||
<h2 class="_style-guide--title">All Content Types</h2> | ||
{% from "components/card.html" import card %} | ||
<div class="card--group"> |
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.
We need a sample with only a single card, and we need a separate template (views/layouts/card
) that is for the layout. They should be displayed separately in the style guide.
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.
Layout needs to match the file name (and therefore the layout name) in the styling
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.
Created the layouts
stuff, but I'm not sure I'm calling it right in styles.html
. I gave it a try and you can tell me if it's wrong.
@@ -0,0 +1,12 @@ | |||
{% macro card(data) %} |
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.
Needs to ultimately match the component name above
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.
ok
@Snugug - got all your changes in, but wasn't sure about the |
@@ -0,0 +1,12 @@ | |||
{% macro basicCard(data) %} | |||
<article class="_basic-card basic-card" itemscope itemtype="http://schema.org/Thing"> |
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.
Need to separate layouts from components. The component should only have the basic-card
class on it, and should be able to be put anywhere. A separate layout should be created, top element with class _basic-card
, that can have the components loaded in to
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.
ok
@@ -0,0 +1,7 @@ | |||
{% from "components/basic-card.html" import basicCard %} |
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.
Until embed lands, the best way to create a sub-layout is to create an include, like you are, but feed data in to it via {% set %}
before it gets called, and have that be used generate the content, looping over the set content.
Here, you're hard-coding this layout to only use the Style Guide's sample data, which we don't want to do.
(Twig's embed function, for reference)
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 gave this a shot. The routes are not setup yet, so I wasn't sure what exactly to do here. I looked at a portion of the products
view
that used {% set %}
as a reference, but it breaks locally so I'm sure it's not right.
@@ -1,5 +1,6 @@ | |||
.basic-card { | |||
border: setting-get('input border width') solid setting-get('secondary color'); | |||
padding: setting-get('rhythm') / .5; |
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 we've discussed, don't divide by a decimal when you should be multiplying instead. This should be multiplied by 2
- Do we not have a double rhythm setting already? If not, we should
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.
Ah, ok. We don't have double rhythm
set, but I will add it.
@@ -1,7 +1,15 @@ | |||
{% from "components/basic-card.html" import basicCard %} |
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 just remove this file for now.
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.
OK. I will write a layout story to tackle this then.
@Snugug - is this ready to merge or are there further changes? |
Was waiting on the update to pass. Waiting on that again. Will merge once passed |
A new component to display content types that include the title, description, and both
Add
andView
action links.Resolves #249
DCO 1.1 Signed-off-by: NICK TILDEN <NICK@INEEDSUBSTANCE.COM>