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

Theme #13

Merged
merged 5 commits into from May 15, 2013
Merged

Theme #13

merged 5 commits into from May 15, 2013

Conversation

@awolfe76
Copy link

@awolfe76 awolfe76 commented May 10, 2013

Contributing the best way I know how. Hope you don't mind.

Created a new theme for the site, nothing major, just maybe a little more readable, with a mobile first approach.

I think I caught everything, a lot more content then I initially realized.

Nice work by the way!

@benbalter
Copy link
Contributor

@benbalter benbalter commented May 10, 2013

@andrew-wolfe Thanks for taking the time to package as a Pull Request. Are you able to attach a few screenshots to this thread by chance? It would really help others join the discussion without having to spin things up locally. You should just be able to drag and drop into the comment field.

@benbalter
Copy link
Contributor

@benbalter benbalter commented May 10, 2013

Also, looping in @dannychapman who was the mastermind behind the first pass.

@awolfe76
Copy link
Author

@awolfe76 awolfe76 commented May 10, 2013

Good idea. Here are 2, the home page. Hopefully enough to start the conversation, but if we need more can definitely add them.

mobile

desktop

@benbalter
Copy link
Contributor

@benbalter benbalter commented May 10, 2013

Wow. This looks awesome. There were a handful of formatting tweaks in main.css for the policymemo... did those make it over by chance (or something similar)?

@benbalter
Copy link
Contributor

@benbalter benbalter commented May 10, 2013

also cc @project-open-data/project-open-data-web ... any thoughts on the above proposed design improvements?

@awolfe76
Copy link
Author

@awolfe76 awolfe76 commented May 10, 2013

Yeah, I brought them over. Seem right? I'm pretty sure I caught everything.

And that's a really handy way to point to specific lines ... that I didn't know you could do. So thanks for that too!

@waldoj
Copy link

@waldoj waldoj commented May 10, 2013

Having a responsive design is a huge win. I see no reason not to merge this.

@awolfe76
Copy link
Author

@awolfe76 awolfe76 commented May 10, 2013

And would you guys rather receive pull requests with a branch or on master? I branched locally but merged before submitting the request.

@benbalter
Copy link
Contributor

@benbalter benbalter commented May 10, 2013

Spun it up locally and clicked around – awesome, awesome work. ❤️

Would love to hold off for a bit to give @dannychapman a chance to weigh in if he wants, but otherwise 👍 from me.

@awolfe76 pointing at master here is the right way to go. Generally speaking, best practices dictate that you'd make a feature branch on your fork, and submit the pull from your feature branch to master of the original repo. That way, your master stays clean, and if while this Pull Request is pending, you want to submit another pull request, you can easily do so by re-branching off of master with a new feature branch and can submit as they can easily be merged down the line.

@@ -1,6 +1,6 @@
#document settings
title: Project Open Data
description: "Open Data Policy — Managing Information as an Asset"
desc: "Open Data Policy — Managing Information as an Asset"

This comment has been minimized.

@benbalter

benbalter May 10, 2013
Contributor

What's the reason for the change here?

This comment has been minimized.

@awolfe76

awolfe76 May 10, 2013
Author

Nothing really. It wasn't being used in the template before, and I just don't like to type whole words. :)

<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
<meta name="description" content="{{ site.description }}">

This comment has been minimized.

@benbalter

benbalter May 10, 2013
Contributor

If the _config.yml change was intentional, would need a matching change in the template.

This comment has been minimized.

@awolfe76

awolfe76 May 10, 2013
Author

Missed that one, changing.

This comment has been minimized.

@benbalter

benbalter May 10, 2013
Contributor

I think description is more, well descriptive. Unless there's a compelling reason to s/description/desc/? I'm open.

}
{% include css/bootstrap.min.css %}
{% include css/bootstrap-responsive.min.css %}
{% include css/main.css %}

This comment has been minimized.

@benbalter

benbalter May 10, 2013
Contributor

In hindsight, 👍 on simplifying this. Good call.

This comment has been minimized.

@awolfe76

awolfe76 May 10, 2013
Author

It was making it all hard to debug/clean the css. It did save on the # of requests though, so I like the idea and will think about how to get the best of both worlds.

catalog.md Outdated
@@ -7,7 +7,7 @@ filename: catalog.md

This section provides further guidance and explanation for implementing the agency data catalog.

/Data Requirements
Data Requirements

This comment has been minimized.

@benbalter

benbalter May 10, 2013
Contributor

@gbinal I Believe you were the one that originally scoped this out... how do you feel about dropping the / from /data?

This comment has been minimized.

@awolfe76

awolfe76 May 10, 2013
Author

Oh, I just thought it was a typo ... my fault, will add it back.

This comment has been minimized.

@benbalter

benbalter May 10, 2013
Contributor

@gbinal can weigh in, but I believe the intention was that it would refer to "slash data" as in agency.gov/data.

@awolfe76
Copy link
Author

@awolfe76 awolfe76 commented May 10, 2013

If we go at it again I'll submit with a branch for sure.

@parkr
Copy link

@parkr parkr commented May 10, 2013

👍 This looks great.

@dannychapman
Copy link

@dannychapman dannychapman commented May 10, 2013

Looks good to me - thanks for looping me in @benbalter! Great work @awolfe76. Let's merge.

@benbalter
Copy link
Contributor

@benbalter benbalter commented May 10, 2013

👍

@benbalter
Copy link
Contributor

@benbalter benbalter commented May 10, 2013

@dannychapman Don't know that FLOTUS is going to be launching a "let's merge" campaign any time soon... but pull requests welcome. :trollface:

@awolfe76
Copy link
Author

@awolfe76 awolfe76 commented May 10, 2013

@benbalter @parkr @dannychapman Thanks!

And just realized @andrew-wolfe, yep that's me and @awolfe76, yep that's me too. 😕 Sorry about that.

"Let's Merge" - I do feel a campaign coming on (and a great geek pickup line).

@stevenvdc
Copy link
Contributor

@stevenvdc stevenvdc commented May 15, 2013

@awolfe76 Thanks so much for the contribution to Project Open Data! The new theme looks great-- love the responsive design aspects. Merging your pull request now... 🇺🇸

- Steve VanRoekel, US Chief Information Officer

stevenvdc added a commit that referenced this pull request May 15, 2013
@stevenvdc stevenvdc merged commit 5e6572d into project-open-data:master May 15, 2013
@ErieMeyer
Copy link
Contributor

@ErieMeyer ErieMeyer commented May 15, 2013

So awesome

@benbalter
Copy link
Contributor

@benbalter benbalter commented May 15, 2013

standing-ovation

@andrew-wolfe
Copy link
Contributor

@andrew-wolfe andrew-wolfe commented May 15, 2013

@stevenvdc Thank you. Looking forward to more from Project Open Data. A great first step.

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

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.