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

Migrate some content to page resources #109

Closed
wants to merge 10 commits into from
Closed

Conversation

RealOrangeOne
Copy link
Member

@RealOrangeOne RealOrangeOne commented Feb 20, 2018

I don't want to move all the images over to bundles yet, as we'd end up with duplicate content, as some images are used on multiple pages. It's not yet possible to access static/ as resources, and headless bundles aren't very nice IMO. In later releases, i'll swap the rest of the content over to resources, with some helpful shortcodes obviously 🎉

@trickeydan
Copy link
Contributor

trickeydan commented Feb 20, 2018

Have we considered using Hugo's shortcodes with the figure element instead of resource_img?

  • EDIT: I have just read your comment mentioning bundles and realise upon reading about them that these are better than using shortcodes.

Copy link
Contributor

@trickeydan trickeydan left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉

@kierdavis
Copy link
Member

What's the reason for upgrading Hugo? I'm assuming it's something along the lines of us wanting to use a feature or bugfix that was introduced in this version.

@RealOrangeOne
Copy link
Member Author

The main feature is page bundles, which means we can put static assets with their content to better group things. It also supports image resizing / cropping etc, but that requires moving images into page bundles, which would create duplicate content.

There's an open issue tracking this feature, and some preliminary work gohugoio/hugo#4381.

Obviously there's other things like additional performance, bug fixes, things like that. And of course, who doesn't like new shiny things!

Copy link
Member

@kierdavis kierdavis left a comment

Choose a reason for hiding this comment

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

Sounds good, and the changes look generally fine to me. Unfortunately a couple of the links give 404s at the moment (see comments).

[small-marker-pdf]: /docs/small-tags.pdf
| PDF | Marker Size | Paper Size |
|---------------------------------|-------------|------------|
| [Large markers](large-tags.pdf) | 250x250mm | A3 |
Copy link
Member

Choose a reason for hiding this comment

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

👎 This link, and the one below, both give a 404 on the Netlify preview.

@@ -5,4 +5,4 @@ weight: 1

You can find the rules for SourceBots 2018 below:

# [Download the rules!](/rules.pdf)
# {{% resource_link src="rules.pdf" %}}Download the rules{{% /resource_link %}}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -52,4 +52,5 @@ robot, exactly 1 metre away.
The following diagram shows the orientation of the Cartesian axes relative to
the camera as well as the angles which describe the spherical coordinate space.

![A diagram showing the coordinate spaces](/img/api/coordinate-spaces.svg)

{{% resource_img src="coordinate-spaces.svg" alt="A diagram showing the coordinate spaces" %}}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -34,7 +34,7 @@ The case measures 70x84x20mm. Don’t forget that the cables will stick out.
## Designs
You can access the schematics and source code of the firmware on the motor board in the following places. You do not need this information to use the board but it may be of interest to some people.

- [Full Schematics](/docs/motor-schematic.pdf)
- {{% resource_link src="motor-schematic.pdf" %}}Full Schematic{{% /resource_link %}}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -63,6 +63,6 @@ The case measures 83x99x24mm. Don’t forget that the cables will stick out.
## Designs
You can access the schematics and source code of the firmware for the power board in the following places. You do not need this information to use the board but it may be of interest to some people.

- [Full Schematics](/docs/power-schematic.pdf)
- {{% resource_link src="power-schematic.pdf" %}}Full Schematic{{% /resource_link %}}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@kierdavis
Copy link
Member

Huh, after more investigation this seems to actually be another case of links behaving differently based on whether there's a slash at the end of the URL or not.

The links at the page work fine: https://deploy-preview-109--sourcebots-docs.netlify.com/markers/
But links on this one give 404s: https://deploy-preview-109--sourcebots-docs.netlify.com/markers

Copy link
Contributor

@PeterJCLaw PeterJCLaw left a comment

Choose a reason for hiding this comment

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

While I appreciate the intent, I'm not completely sold on the idea of moving our images next to their source files (both generally and because of the following specific reasons).
As has already been noted, this has issues where the same resource image wants to be used multiple times.
A consequence of that is that (as I understand it), we'll end up with two classes of place where images could be: by the content file which references them, and in the /static folder for images used more than once. This seems likely to cause more problems than it solves as it won't be clear where to look for suitable images, or where to put new ones.

Meta:

  • why are we moving around quite so much of the content while upgrading our build engine? Doing so makes it hard to validate that the upgrade hasn't broken anything, while also making it more complex for future readers of the merge to understand what the changes were.
  • why is the theme being updated at the same time? Is this needed to enable the upgrade?

@@ -25,9 +25,3 @@ The markers in the list have some useful attributes:
{{% notice tip %}}
A marker's position can be represented using both the [Cartesian](coordinates/#cartesian-coordinates) and [spherical](coordinates/#spherical-coordinates) coordinate systems.
{{% /notice %}}

## Marker PDFs
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing this? This is a potentially breaking change for people used to coming to this page for these links.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's removed mostly to remove the duplicated content. Perhaps adding a link would've been better? The markers are already a top-level section (theyre in the navbar), so finding the links isn't difficult

@@ -8,4 +8,4 @@ rm -rf public/
rm -rf content/tutorials/kit-assembly.files
cp -r static/img/assembly content/tutorials/kit-assembly.files

hugo -v
hugo -v --gc
Copy link
Contributor

Choose a reason for hiding this comment

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

Curiosity: what does --gc do?

Copy link
Member Author

Choose a reason for hiding this comment

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

It removes the build cache of static files before building the site. Resizing images is fairly trivial, and this removes any issues which may come from changes in the resizing

@RealOrangeOne
Copy link
Member Author

@PeterJCLaw makes some good points on file duplication in this. I didn't realise when I started how much there would be. I think it's worth revisiting this when the required updates to Hugo are made to support this, and we can properly use resources from a single location (in the cases it's needed), and not fix content to only being accessible from a single page.

(This is sort of already implemented in Headless Page Bundles, but i'm not a fan of that mechanism for solving this, so it's probably better to wait for a better solution.

@RealOrangeOne RealOrangeOne changed the title Hugo upgrade Migrate some content to page resources Feb 21, 2018
@RealOrangeOne RealOrangeOne added the Frozen Won't fix this issue label Feb 21, 2018
@PeterJCLaw
Copy link
Contributor

@RealOrangeOne is there anything holding us back from upgading hugo anyway? I realise that we might not (currently) need any of the latest features, but if we've already validated that it works then there might be benefit from doing so.

Related question: what's the support lifecycle of a hugo release? I realise they're still very much in the move-fast-and-break-things stage (0.x), but are they trying to keep it easy to upgrade between adjacent releases? releases, say, 5 apart?

@RealOrangeOne
Copy link
Member Author

@PeterJCLaw hmm, perhaps i'll strip out the resource changes from this PR and ship.

The releases currently may contain breaking changes, but all the time i've been using it, there's only been a handful, if that. As it's currently still technically in beta, that's to be expected. I think it's fine to be upgrading and fixing things as and when new versions come out (not every version, they happen incredibly often), but as you say every 5 releases or so perhaps

@PeterJCLaw
Copy link
Contributor

@RealOrangeOne it might be easier to put up a separate PR for the upgrade than to try to munge this one.

@RealOrangeOne RealOrangeOne mentioned this pull request Aug 1, 2018
@PeterJCLaw
Copy link
Contributor

I'm closing this PR in favour of #147 (which declares that it replaces it).

@PeterJCLaw PeterJCLaw closed this Aug 4, 2018
@RealOrangeOne RealOrangeOne deleted the hugo-upgrade branch August 4, 2018 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frozen Won't fix this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants