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

Better press page #196

Merged
merged 36 commits into from
Oct 3, 2018
Merged

Better press page #196

merged 36 commits into from
Oct 3, 2018

Conversation

dirtyredz
Copy link
Contributor

Description of changes

Adds "In The News" section to the Press page.
Fixes minor styling issues.
Updates testing for common button component for handling onClick.

Issue Resolved

Fixes #171
As it now adds the missing article links.

Screenshots/GIFs

inthenews
inthenewsexpanded
inthenewsresponsive1
inthenewsresponsive2

@vercel
Copy link

vercel bot commented Oct 3, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@dirtyredz
Copy link
Contributor Author

Looks like a few updates have been made since ive opened this branch.

@kylemh
Copy link
Member

kylemh commented Oct 3, 2018

🤖 This is a bot 🤖

🎉 Deployed Storybook preview! 🎉

Click the link at the bottom of this comment to see it live.

Built with commit cc2c97d

https://deploy-preview-196--operation-code-storybook.netlify.com

@dirtyredz
Copy link
Contributor Author

Styles were pulled from OperationCode/operationcode_frontend#1024

@kylemh
Copy link
Member

kylemh commented Oct 3, 2018

The conflicts are going to be pretty rough to resolve 😥

I recommend closing this PR and copy-pasting to the correct spots, as I moved some folders around (I simply wanted to modularize content).

Thanks so much for working to simplify this! I love the direction your going.

When you're done porting over, you can ignore test coverage for the data structure containing all the links via changing jest.config.js - there's no logic to test there.

@dirtyredz
Copy link
Contributor Author

dirtyredz commented Oct 3, 2018

Will Do

@dirtyredz
Copy link
Contributor Author

@kylemh Merged/Updated and moved the new PressLinks into the Press directory. should be ready for a merge.

Copy link
Member

@kylemh kylemh left a comment

Choose a reason for hiding this comment

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

Amazing work. Solid test coverage and it looks super clean.

Mostly variable naming changes and some edits in the testing. I gave you the tools needed to accomplish your UI for the "show all" button.

@dirtyredz
Copy link
Contributor Author

All make these fixes asap, and thanks for taking the time to explain yourself on the changes. It helps me better understand what your wanting, making it easier for me to help you guys out.

@kylemh
Copy link
Member

kylemh commented Oct 3, 2018

No problem at all @dirtyredz - hopefully it's useful info. Feel free to @ me on Slack at any moment. Let's get you a shirt in addition to those stickers 😉

@dirtyredz
Copy link
Contributor Author

dirtyredz commented Oct 3, 2018

just need to work on the button aria portion

@dirtyredz
Copy link
Contributor Author

There we go, that should resolve everything. Again thanks for the collaboration very helpful!!

Copy link
Member

@kylemh kylemh left a comment

Choose a reason for hiding this comment

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

Just one more switch + remove that snapshot test

this is HUGE FOR US 😁

@kylemh
Copy link
Member

kylemh commented Oct 3, 2018

I made some commits to make small edits to things I didn't wanna bother you about - be sure to pull and update snapshot tests - the build should resolve again

@dirtyredz
Copy link
Contributor Author

ugh, I ALWAYS forget to update component names... lol, thanks for those fixes. Ill pull and resolve the remaining reviews.

Copy link
Contributor Author

@dirtyredz dirtyredz left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor Author

@dirtyredz dirtyredz left a comment

Choose a reason for hiding this comment

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

... I was totally being lazy one this one, cant get nothing past you.

Copy link
Member

@kylemh kylemh left a comment

Choose a reason for hiding this comment

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

Incredible work.

@dirtyredz
Copy link
Contributor Author

Thank you, and great job yourself keeping me on my toes.

@dirtyredz
Copy link
Contributor Author

Tonight ill do a PR for updated Linting rules specifically alpha order and imports.

@kylemh
Copy link
Member

kylemh commented Oct 3, 2018

@dirtyredz less about keeping you on your toes and more about me being nitpicky.

Thanks ahead of time for doing that issue!

@kylemh
Copy link
Member

kylemh commented Oct 3, 2018

@dirtyredz will you do me the favor of pulling changes and pushing snapshot updates?

@dirtyredz
Copy link
Contributor Author

sure

@kylemh kylemh merged commit 8e89c38 into OperationCode:master Oct 3, 2018
@dirtyredz dirtyredz deleted the BetterPressPage branch October 3, 2018 23:44
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.

2 participants