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

Hide github icon unless on developer page #41

Closed

Conversation

DiscoStarslayer
Copy link
Contributor

Closes #39

Option 1 (this pr)

Overrides to the existing partials so that the github icon is not shown on any pages other than the developer pages.

I like that this implementation keeps the conditional display of the github icon in the templates, statically rendering to html.

I don't like how much code needs to be repeated:

Option 2

I also have an alternative implementation done with JS + CSS: https://github.com/DiscoStarslayer/xqemu.com/pull/1/files

I feel the intent of the code is clearer on the JS implementation, but I am not a big fan of the fact that it has to run in the browser instead of just being a statically rendered element.

Option 3

Just remove this icon all-together to avoid the added complexity. One line of CSS + a config change to mkdocs.yml will do this pretty cleanly.

@JayFoxRox
Copy link
Member

I vote to remove the GitHub logo from the theme altogether.

On IRC, there have been concerns about us removing the GitHub logo, but this change is mostly to avoid users coming to GitHub, thinking it's a forum. We'll still prominently feature our open-source background; probably even bigger than the current logo.

I believe we should prominently display it on the "Welcome Developers" page (GitHub logo, repo link and CI status). Also see #43 .

@DiscoStarslayer
Copy link
Contributor Author

I agree with removing entirely, as it reduces complexity of the implementation somewhat.

As for implementation direction:

  1. should we go with overriding the partials (adding two HTML files to a overrides directory, like done in this PR)
  2. Hiding the github element with CSS

1 is cleaner from the user perspective, as we don't ship them un-used HTML, but less clean from a developer position as we need to override the theme partials and it's not immediately clear why from just looking at the code.

2 is much cleaner from a code perspective, as it's a single line change, and obvious what it is doing. But we are shipping HTML to the user that they will never see so it feels kinda klunky.

There is also a third option, looking at the existing partials, it seems to suggest that simply removing the repo_url config will remove the github links. In practice, this does not work, as removing the config leaves a bunch of white space where the github links used to be:
Screen Shot 2019-05-30 at 9 41 31 AM
Screen Shot 2019-05-30 at 9 41 40 AM

Maybe it's worth trying to upstream a PR to fix this bug in the theme so we can keep our own codebase clean?

@JayFoxRox
Copy link
Member

Yes. Removing repo_url should work (I thought it worked fine in the past?). It's also my preferred method. Sounds like an upstream issue.

@GXTX
Copy link
Contributor

GXTX commented Oct 2, 2019

I don't think this is a good idea. I often find myself quickly viewing a projects website for the sole purpose of looking for their Github.

@DiscoStarslayer
Copy link
Contributor Author

Fair comment.

I think a counter to your point is this doesn't remove the github link entirely. The developer section has a link to the github repo prominently on the page, it also details important context for any potential contributor (XboxDev group ect.)

I think this page is more valuable than the github repo by itself, so this change will help reduce non-developer users from hitting the github repo (poor experience, multiple screen pages to readme, readme not the most descriptive/useful for user). I also think this will improve the experience for potential contributors interested in the project as they are provided context before being routed to the repo which is pretty barren in this regard.

@JayFoxRox
Copy link
Member

What shall we do with this?

@JayFoxRox
Copy link
Member

I'll close this now. It's currently not going anywhere.
Please create a new PR if this is being worked on again.

Closed.

@JayFoxRox JayFoxRox closed this Mar 23, 2020
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.

GitHub icon should not appear on user pages
3 participants