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

Reimplement /press page #171

Closed
kylemh opened this issue Oct 1, 2018 · 12 comments
Closed

Reimplement /press page #171

kylemh opened this issue Oct 1, 2018 · 12 comments
Assignees
Labels
beginner friendly A relatively new developer should be able to accomplish this task good first issue If somebody is new to this repository, this is a simple way to dive in. Type: Feature/Redesign

Comments

@kylemh
Copy link
Member

kylemh commented Oct 1, 2018

Feature

Why is this feature being added?

https://operationcode.org/press exists and it needs to exist in the new code base as well.

What should your feature do?

  • Implement the page via pages/press.js
  • See live source code. Feel free to copy-paste a lot.
  • Refactor as required to match our new standards (linting, using components where possible, etc.)
  • Add new content not on site! A list of news articles under a section titled "In The News". Put every article listed in these issues in the list: one, two, and three.
@kylemh
Copy link
Member Author

kylemh commented Oct 1, 2018

This is a request for a new or redesigned page.

  • The page should be implemented by creating a file (or editing the file if it already exists) in the pages folder. The file name should match the route (ex: code_schools --> pages/code_schools.js).
  • Please remember to leverage the <OutboundLink> component when dealing with external links (leaving our hostname - operationcode.org)
  • Do your best to limit stylesheets imported into pages/styles.
  • Only modularize potentially reusable content via components - it's okay to implement everything in the page file and let us as staff members sort out where you could write for reuse (if anywhere).
  • If no design is missing mobile-specific or desktop-specific styling, do your best to work with what's given. A good rule of thumb is that any lists displayed in rows on desktop mocks, can be displayed one-per-row on mobile.

@dirtyredz
Copy link
Contributor

My Pr should not close this issue, its only a copy of the live site. No new content has been added.

@kylemh
Copy link
Member Author

kylemh commented Oct 2, 2018

Thanks to @dirtyredz a ton of the work is already done. The issue still needs the newly requested content + some style tweaks.

@dirtyredz
Copy link
Contributor

Do we have a new design template? So i can look into tweaking styles and more specifically where were looking at adding the above referenced media articles.
I don't want do a bunch of styling work if there is already a planned redesign available.

@kylemh
Copy link
Member Author

kylemh commented Oct 2, 2018

There's no mock to speak of here.

Some of the style issues I noticed, if you'd like to tackle them:

  • Videos aren't centered horizontally
  • Videos and Branding sections should have a theme prop value of "grayLight"
  • Photos aren't within a flex container / centered horizontally
  • The CivixX OutboundLink should have a hasIcon prop value of "false"

@dirtyredz
Copy link
Contributor

The horizontal centering doesn't look terribly off but notice it the most on lower device sizes. Ill see what can be done for those. The other issue look fairly straight forward.
Ill work on these tonight, Thanks

@kylemh
Copy link
Member Author

kylemh commented Oct 2, 2018

I wonder if we're seeing different things.

I have this view:
screen shot 2018-10-02 at 1 24 27 pm

@kylemh
Copy link
Member Author

kylemh commented Oct 2, 2018

To be clear, I wasn't being critical of your work. It's actually a very strange issue, since you've simply brought over the old code and there shouldn't be styling regressions. Thanks again for doing so much!

@dirtyredz
Copy link
Contributor

I understand, i figured it was a matter of browser or device that was causing the distinct difference.
Your screenshot helps me identify what you were seeing, and I should have that fixed with my next pr.

@kylemh
Copy link
Member Author

kylemh commented Oct 2, 2018

Chrome@latest for what it's worth

@dirtyredz
Copy link
Contributor

This latest PR should close out this issue. A few merge conflicts need to get fixed but doesn't look like anything major.

@kylemh kylemh self-assigned this Oct 3, 2018
@kylemh
Copy link
Member Author

kylemh commented Oct 3, 2018

Assigning myself as a placeholder for @dirtyredz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginner friendly A relatively new developer should be able to accomplish this task good first issue If somebody is new to this repository, this is a simple way to dive in. Type: Feature/Redesign
Projects
None yet
Development

No branches or pull requests

2 participants