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

New OldNew Mashup Theme #713

Closed
cshoredaniel opened this issue Sep 25, 2019 · 13 comments

Comments

@cshoredaniel
Copy link

@cshoredaniel cshoredaniel commented Sep 25, 2019

OldNew Mashup theme submission

This basically a complete rewrite of the old OldNew Mashup theme I asked you to remove. It's much, much better, validates against W3C org's tools, and looks better.

I hope you like if you get a chance to check it out!

NB The License file I used is LICENSE not LICENSE.md, however I have an appropriate licenselink in theme.toml and testing using the theme site generateThemeSite and reveals that the link works as expected (goes to GitHub repo's copy of LICENSE).

Also, the theme works as expected with the hugoBasicTheme, but the content under exampleSite shows off more features of the theme. I have a demo site (linked as the theme homepage) for the theme so it's not essential that exampleSite be used.

Link to my theme repository: https://github.com/cshoredaniel/new-oldnew-mashup/tree/stable-0.9

I made sure that...

  • the repository contains a good README.md describing my theme
  • an open source license has been added to LICENSE
  • all metadata have been added to theme.toml
  • screenshots have been added in the images/ folder with the required dimensions
  • in case I'm using a customized demo via the exampleSite folder that
  • I tested my theme against the gohugoio/HugoBasicExample
    • I've checked the developer tools' console in my browser for error messages
  • in case my theme is using Hugo Pipes features like toCSS and PostCSS that I have committed the /resources directory with all generated assets, for my theme to work in the basic version of Hugo

I'll do my best to keep the theme maintained, or let you know if it no longer is alive.

@digitalcraftsman

This comment has been minimized.

Copy link
Member

@digitalcraftsman digitalcraftsman commented Sep 27, 2019

Hello Daniel,

thank you for making another submission attempt with a revised version of your theme. Let's start straight with the review:

  • Is it possible to create a single branch that gets updated with each new release? Otherwise we would always have to switch the newest stable-{version} branch. Pulling releases from a single, pre-defined branch is much easier in the long run.
  • The search does not work if a website is hosted in subdirectory, such as https://themes.gohugo.io/theme/{some-theme}/. In search.min.js you hard-coded the url to the search index as /index.json. This way you refer to https://themes.gohugo.io/index.json instead of https://themes.gohugo.io/theme/{some-theme}/index.json. A possible work-around is to define the url to the search index as variable in combination with a template function such as absURL:
var url = {{ "index.json" | absURL }};
// include search.min.js afterwards and use the url instead of a hard-coded path
@cshoredaniel

This comment has been minimized.

Copy link
Author

@cshoredaniel cshoredaniel commented Sep 28, 2019

@digitalcraftsman Thanks again for all you hard work on hugo themes repository and especially for the help you have given me in the past (and now).

I've been thinking about the branch thing and decided the best way forward is to make master the release (stable) branch and make the development branch something like devel-<next_version>. I don't remember why I flipped that around (I used to do it that way). It was probably because I got in the habit of master being the current development branch when working on OpenWrt.

On at least some projects I may fork off and oldstable-x.x.x branch when stable changes, but I think user master for the current production version rather than development better suits most expectations and norms.

Anyway, on the search form: Thanks for catching that -- it wasn't even on my radar. The workaround seems quite satisfactory for my needs.

That has, however, reminded me of another things that's not going to work on the theme site, and that is my contact form -- it is designed to contact the same server on a different port and doesn't go out some other site (and I don't want to change that).

Is there a way to not render certain pages only when for the hugo themes site? (Assuming exampleSite is being used and not the hugoBasicExample. If it's just a standard hugoBasicExample, it's non-issue).

@cshoredaniel

This comment has been minimized.

Copy link
Author

@cshoredaniel cshoredaniel commented Oct 7, 2019

I realized for the conditional render I can check baseURL...so I'll try that out this week.

@stale

This comment has been minimized.

Copy link

@stale stale bot commented Oct 27, 2019

This issue has been automatically marked as stale because it has not had recent activity. Feel free to ask questions. We're glad to help.
This issue will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

@stale stale bot added the Stale label Oct 27, 2019
@cshoredaniel

This comment has been minimized.

Copy link
Author

@cshoredaniel cshoredaniel commented Nov 11, 2019

Been busy. Will open a new issue if necessary, once ready.

@stale stale bot removed the Stale label Nov 11, 2019
@cshoredaniel

This comment has been minimized.

Copy link
Author

@cshoredaniel cshoredaniel commented Nov 14, 2019

@digitalcraftsman I believe I've fixed the issues with the search (no longer hard coded, and works using the test instructions in this repo's README). Also I've made master the stable branch (so the URL for the repo is https://github.com/cshoredaniel/new-oldnew-mashup.git . Development will be done on other branches and only merged into stable for releases.
I also fixed a few other nits I noticed, unrelated to the theme site.
If you could kind review again it'd be much appreciated!

@onedrawingperday

This comment has been minimized.

Copy link
Contributor

@onedrawingperday onedrawingperday commented Nov 23, 2019

Hello @cshoredaniel

When I try to test your theme I get the following ERROR

Error: module "github.com/cshoredaniel/necolas-normalize.css" not found; either add it as a Hugo Module or store it in "/home/alex/work/src/hugoThemes".: module does not exist

It seems that you need to configure that module again.

@cshoredaniel

This comment has been minimized.

Copy link
Author

@cshoredaniel cshoredaniel commented Nov 27, 2019

Hmmm... I wonder if it's related the version of Hugo. My CI tests start from scratch on Travis and succeed, but I've been using an older version because there was a version that was segfaulting in the Travis Bionic environment (I believe I filed a bug about that). I guess it time to try the latest version and do necessary updates.

@onedrawingperday

This comment has been minimized.

Copy link
Contributor

@onedrawingperday onedrawingperday commented Nov 27, 2019

@cshoredaniel
The themes repo always uses the latest Basic version of Hugo, so it's advisable to check your theme with the latest.

I am not sure whether the ERROR is caused due to the way that module is named i.e. necolas-normalize.css. The .css extension may be causing the issue, but I'm not sure, that's just a hunch, but if it's true, then probably a bug report may need to be filed at https://github.com/gohugoio/hugo/issues

@cshoredaniel

This comment has been minimized.

Copy link
Author

@cshoredaniel cshoredaniel commented Nov 29, 2019

Ok, I've updated the theme (it now only uses Hugo modules, no more git submodules). I didn't need to quite do do that (the real issue was some difference in the generated hash ca. 0.58.0 or .59.0 compared to earlier versions; no bug to report; also the Travis CI tests now work, so the reason I was sticking with 0.57.2 has been resolved).

I did notice that the generateTheme.sh results in the use of my exampleSite instead of basicTheme -- either will work, but I wanted to make sure that was intentional and not because of some issue with generateTheme.sh script.

I hope you like the new theme (and it's another theme that is using Hugo modules that folks can look at, if you think it worthwhile.

Oh, and I caved and added back at least rudimentary IE11 support -- I was trying to drop it before, but my stats show too many people trying to access my main website with IE11 for that to work. Pity.

@onedrawingperday

This comment has been minimized.

Copy link
Contributor

@onedrawingperday onedrawingperday commented Nov 30, 2019

I did notice that the generateTheme.sh results in the use of my exampleSite instead of basicTheme -- either will work, but I wanted to make sure that was intentional and not because of some issue with generateTheme.sh script.

That's because you have mounted the exampleSite's content using Hugo Modules.
Currently the Build Script does not enforce the hugoBasicExample through Hugo modules and as a result the override does not work in your case.

However your theme's previous version was whitelisted so that the demo could render fine and I am fine with having your provided exampleSite used in the demo.

If the Build Script gets adapted to take into account Hugo Modules we will add your theme in the whitelist again.

I have added the theme in the repo in 3f14ab9


@digitalcraftsman

The new version of the throwback Old New Mashup theme has now been added to the Themes Showcase.

@cshoredaniel

This comment has been minimized.

Copy link
Author

@cshoredaniel cshoredaniel commented Dec 1, 2019

Thank you for all your help, and adding my theme.

@digitalcraftsman

This comment has been minimized.

Copy link
Member

@digitalcraftsman digitalcraftsman commented Dec 3, 2019

@cshoredaniel I've promote your theme on Hugo's official Twitter account.

@onedrawingperday thanks for your support in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.