Skip to content
This repository has been archived by the owner on Jun 10, 2019. It is now read-only.

Fixes #1017: Adds media highlights to Operation Code website: General #1024

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cyph3r-m00n
Copy link
Contributor

Description of changes

Adds Media Highlights to Press Page

Issue Resolved

Fixes #1017

@cyph3r-m00n cyph3r-m00n closed this Aug 6, 2018
@cyph3r-m00n cyph3r-m00n reopened this Aug 6, 2018
@kylemh
Copy link
Member

kylemh commented Aug 6, 2018

Thanks so much for your continued contributions! For all of those anchor tags, we have an OutboundLink component that ties into our analytics. Could you switch to those?

@cyph3r-m00n
Copy link
Contributor Author

@kylemh For sure! I'll Check that out now!

@cyph3r-m00n
Copy link
Contributor Author

@kylemh All of the Anchor tags have been changed to use the OutboundLink component!

@kylemh
Copy link
Member

kylemh commented Aug 6, 2018


@media screen and (min-width: 415px) and (max-width: 1280px) {
.logos > .imgBox {
width: 350px;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any elements with the class .imgBox in the markup - so is there any need for this CSS rule?

>
Operation Code Looks to Help Veterans Land IT Careers
</ReactGA.OutboundLink>
</li>
Copy link
Member

Choose a reason for hiding this comment

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

selection_081

The list-items could use some vertical space between them on mobile to provide visual separation between elements. I suggest adjusting the line-height for the all <li> elements, and adjusting the margin-top for adjacent sibling <li> elements:

.item {
  line-height: 1;
}

.item + .item {
  margin-top: 5px;
}

Due to the small width of mobile screens, for the <ul> element, the bullets can probably be removed, and the padding can be adjusted:

.list {
    list-style: none;
    padding: 0 20px;
}

Copy link
Member

@jjhampton jjhampton left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the contrib @cyph3r-m00n ! Only a few minor changes requested - please see comments that @kylemh and I already posted.

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

Successfully merging this pull request may close these issues.

Add media highlights to Operation Code website: General
3 participants