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

Feature/next/article #263

Merged
merged 21 commits into from
May 24, 2019
Merged

Feature/next/article #263

merged 21 commits into from
May 24, 2019

Conversation

dw-buildit
Copy link
Contributor

@dw-buildit dw-buildit commented May 17, 2019

This PR is two components and some global style updates.

The two components are:

Page Intro
To be used as the <heading> for most of our pages. It includes the heading, description and
optional time and image as per reference designs.

Ostentatious Copy
This is expected to contain copy that you want to look a little more showy. Generally adds extra padding between elements and around the component itself. Pulls pull quotes out (margin wise) and on larger screen sizes indents a lot from the left.

Style Updates
The PR also includes updates to many text element, list element and some padding styles. Again to better reflect the reference design.

Also
There's styling for the <hr> element.
There's a page for viewing all the article components with semi-realistic data.

@dw-buildit dw-buildit added the WIP STATUS: Work in Progress - Do Not Merge. Useful for long-running PRs label May 17, 2019
@dw-buildit dw-buildit added this to the Gravity NEXT milestone May 17, 2019
@dw-buildit dw-buildit changed the base branch from develop to next May 17, 2019 15:20
@ashblue
Copy link
Contributor

ashblue commented May 20, 2019

For the 2nd commit feat(notch decoration): added parameters to the mixin is that really a feature? I feel like it should fall under a chore or refactor because it isn't declaring anything new for the repository.

@james-nash
Copy link

Just adding a cross reference to issue #178, since this partially covers that.

Partially, since we don't yet have provisions for article author, tags and/or categories. However, all of those could be added in a future PR, after the website MVP is complete.

@dw-buildit dw-buildit force-pushed the feature/next/article branch 2 times, most recently from 21eaa67 to ae72030 Compare May 22, 2019 15:59
@dw-buildit
Copy link
Contributor Author

For the 2nd commit feat(notch decoration): added parameters to the mixin is that really a feature? I feel like it should fall under a chore or refactor because it isn't declaring anything new for the repository.

I did think that this was a feature that developers of Gravity would be able to use (dynamic notch size/position) although I wouldn't actually expect any users to use it. I've changed this commit's message anyway 👍

@dw-buildit dw-buildit force-pushed the feature/next/article branch from 79fa7ed to 8f7716c Compare May 23, 2019 11:59
@dw-buildit dw-buildit removed the WIP STATUS: Work in Progress - Do Not Merge. Useful for long-running PRs label May 23, 2019
@dw-buildit dw-buildit force-pushed the feature/next/article branch 2 times, most recently from 6ca636d to a2f9aec Compare May 23, 2019 16:25
@dw-buildit dw-buildit marked this pull request as ready for review May 23, 2019 16:26
@dw-buildit dw-buildit requested review from ashblue and james-nash May 23, 2019 16:26
Copy link

@james-nash james-nash left a comment

Choose a reason for hiding this comment

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

Lookin' pretty good! Just a few minor tweaks...


This is the component for use as the page heading.

It consists of a `<h1>` as the page title, a `<p>` for a short description of the page, an optional `<time>` to display a date related to this content (usually publication date), and an optional `<img>` which will automatically gain a downward pointing notch.

Choose a reason for hiding this comment

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

Maybe add some guidance here about when to use a <header> element or a <div> to markup the page intro.

</article>
<hr />
<aside class="grav-o-container">
<ul class="grav-c-related-items">

Choose a reason for hiding this comment

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

You should replace this with {% include 'organisms-related-items' %}. That way, if the related items component ever changes, it will be reflected here. Also, it lets Pattern Lab / Fractal create lineage links between those components in the pattern library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely! That was an oversight.

}

@media all and (max-width: grav-breakpoint(medium)) {
padding-right: $grav-sp-s;

Choose a reason for hiding this comment

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

We can avoid this max-width media query. You can set this padding and the blockquote styles outside of the media query.

Then, below, in the min-width media query, where you already alter the blockquote styles as needed, you can update the padding to be large instead of small.

@dw-buildit dw-buildit requested a review from james-nash May 24, 2019 08:47
@dw-buildit
Copy link
Contributor Author

@james-nash CR amends completed.

Copy link

@james-nash james-nash left a comment

Choose a reason for hiding this comment

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

Lurvly

dw-buildit added 10 commits May 24, 2019 13:03
affects: @buildit/gravity-ui-nunjucks, @buildit/gravity-ui-web
affects: @buildit/gravity-ui-web

the mixin now allows you to supply an inset value and a size value. these have to be in the same
unit type
affects: @buildit/gravity-ui-nunjucks, @buildit/gravity-ui-web
affects: @buildit/gravity-ui-nunjucks, @buildit/gravity-ui-web
affects: @buildit/gravity-ui-nunjucks, @buildit/gravity-ui-web
affects: @buildit/gravity-ui-nunjucks

- added related item to page
- added blockquote with citation to copy
@dw-buildit dw-buildit force-pushed the feature/next/article branch from 6bbc2c1 to b35257b Compare May 24, 2019 12:05
@dw-buildit dw-buildit merged commit 9ea4e55 into next May 24, 2019
@dw-buildit dw-buildit deleted the feature/next/article branch May 24, 2019 12:20
@buildit-dev
Copy link

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants