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

add github contribute link to footer of all qunitjs.com pages #189

Closed
wants to merge 1 commit into from

Conversation

johnkpaul
Copy link
Contributor

I've added the "Open an Issue or Submit a Pull Request on GitHub" button to the footer of all qunitjs pages. The footer should look like:
Screen Shot 2013-01-24 at 10 32 06 AM

@scottgonzalez
Copy link
Member

Seems like the icon should have a different color. It also doesn't seem clear to me that this is for reporting issues about the site as opposed to the QUnit code.

@johnkpaul
Copy link
Contributor Author

Hm, yeah, I agree. I just changed the color of the icon to white, but conveying the intention is either a copy or a placement change.

@rdworth
Copy link
Contributor

rdworth commented Jan 24, 2013

How bout 'SITE SUGGESTIONS, PROBLEMS, FEEDBACK?'

@johnkpaul
Copy link
Contributor Author

@rdworth I think that could work. It is definitely more specific

I've tried another option that you can see in this screenshot:
Screen Shot 2013-01-24 at 11 26 51 AM

https://github.com/johnkpaul/web-base-template/compare/add_qunitjs_github_link_2

If we do this though, I need to change the content as well. The internal content pages are broken anyway for me, because the final paragraph tags don't have class "eight columns". Let me know if this is preferable and I'll update this PR.

@rdworth
Copy link
Contributor

rdworth commented Jan 24, 2013

I think it's abundantly more clear when it sits in the content area. @scottgonzalez has expressed hesitation on the feasibility of that in the past. Perhaps he can weigh in.

@RedWolves
Copy link
Member

We should be consistent. The learn site has it in the content area as well.

content feedback

@scottgonzalez
Copy link
Member

Well, it certainly doesn't belong in the raw content. There's a pre-processor available to modify content during the build step, but if it was going to be at the bottom of the content area, then it should just go in the theme anyway. I'm not really sure it belong there though; it looks a bit out of place to me.

@johnkpaul
Copy link
Contributor Author

@scottgonzalez I don't mean in the raw content. I mean in the content area using the theme. I added a custom content page to themes/qunitjs.com

You can see it here https://github.com/johnkpaul/web-base-template/compare/add_qunitjs_github_link_2

@jzaefferer jzaefferer closed this in 34b1299 Feb 4, 2013
@jzaefferer
Copy link
Member

I took the _2 branch and added some margin-top to offset the GitHub aside from the content.

@johnkpaul good stuff. Any idea how to make the same happen for the API site? I tried duplicating the content-page.php file, but that didn't do anything. And content-api.php is just used for the listings, not the individual entries.

@scottgonzalez
Copy link
Member

We should really avoid adding templates like this to specific site themes. If we want consistency, we need to be implementing this kind of thing in the base theme.

@johnkpaul
Copy link
Contributor Author

@jzaefferer It seems to need to be in a different place for each site and to be honest, I don't really understand why. I don't have a good idea of how the child and base themes are put together to form the final site.

I agree with @scottgonzalez, because eventually, we want this on every page of every theme. I just don't know how to accomplish that generically because each site has slightly different styling/column conventions.

jzaefferer added a commit that referenced this pull request Feb 4, 2013
@jzaefferer
Copy link
Member

Reverted the commit. Not sure if reopening is the right thing here. I don't know how the grid system works or where this could be added to do it consistently across all sites.

@jzaefferer jzaefferer reopened this Feb 4, 2013
@ajpiano
Copy link
Member

ajpiano commented Feb 4, 2013

I think putting it in the footer is the most reasonable place for all sites - it doesn't have to stay in the article content for learn site, it can use the area in the footer as well. We'll have to dupe it just for the 4 footers we have, but that is not a big deal. I don't think we will find a "more common" place to put it than that. I think it should probably be its own column in the footer though

@ajpiano
Copy link
Member

ajpiano commented Feb 4, 2013

Ultimately discoverability is an issue if it's at the bottom of the page no matter what, whether in the footer or the bottom of the content. Would be good to find a way to get it "above the fold" per se.. we could go the "Fork on GitHub" route corner ribbon route, perhaps

@johnkpaul
Copy link
Contributor Author

Any thoughts about this? I'd love to get this working in a way that is usable across all of the sites. I just don't know where it belongs from a UX/design perspective.

@scottgonzalez
Copy link
Member

What if we just designed something that would sit in the bottom right corner like kampyle.com?

@johnkpaul
Copy link
Contributor Author

That makes sense to me. It is out of the way of the main information and can be added to every page more easily. How do we get the design process started?

@scottgonzalez
Copy link
Member

We're pretty light on design resources. @kleinmaetschke could you design something for this?

@kleinmaetschke
Copy link
Contributor

Since I'm still fairly new to the github stuff, I have no idea how to "reference this pull request". ..here's my contribution to this need: 753f3ba

@kleinmaetschke
Copy link
Contributor

@scottgonzalez @ajpiano Anyone had a moment yet to look this over?

@jzaefferer
Copy link
Member

@kleinmaetschke GitHub is weird. Took me a while to figure out that that commit is in your repo, not here, as GitHub suggests.

Anyway, I think this looks: whatever
It could be a little smaller. Depending on the screen width, I can't use the "IRC" link in the footer anymore.

Otherwise, there needs to be an actual link, and it can't use inline event handlers. I'm not sure if we can just stick target="_blank" in there or if that's not valid anymore, but something like that along with a proper a-element and href attribute.

@kswedberg
Copy link
Member

@jzaefferer fwiw, regarding the validity of the target attribute, it was deprecated in HTML5, but isn't anymore: http://dev.w3.org/html5/markup/a.html#a.attrs.target

@jzaefferer
Copy link
Member

Thanks Karl, that would explain my confusion. So yeah, one or more a elements with target="_blank".

@kleinmaetschke
Copy link
Contributor

Hey @kswedberg and @jzaefferer I made a pull request with more proper code and a smaller ribbon size that shouldn't overlap with the IRC link.

@Krinkle
Copy link
Member

Krinkle commented Oct 2, 2013

Few thoughts:

  • The capitalisation of the button seems a bit weird: "Open an Issue or Submit a Pull Request on GitHub". Title-casing like that is imho weird in general, but especially with such an extremely long label it isn't very atractive.
  • Both the button and the purple banner concepts are imho still ambiguous. Or actually, it's not ambiguous, I think it's obvious to the user that it means contribution to QUnit itself. Positioning it inside the content-wrapper is imho not better than in the footer. Adding an explicit mention of the word "site" would make it clearer, but it still looks weird to me.
  • Perhaps we can make it appear more content-related by being more explicit in its intention (e.g. "Edit this page"). Positioned in a meta footer at the bottom of the content and/or near the page title.

screen shot 2013-10-02 at 6 50 57 pm

@scottgonzalez
Copy link
Member

Closing due to inactivity. I'd really like to see us move forward with a solution for #279, but this seems to have stalled.

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

Successfully merging this pull request may close these issues.

9 participants