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

ENH: Bring 404 page from lesson example #112

Merged
merged 1 commit into from
Apr 3, 2021
Merged

ENH: Bring 404 page from lesson example #112

merged 1 commit into from
Apr 3, 2021

Conversation

jhlegarreta
Copy link
Contributor

Bring 404 page from lesson example.

@jhlegarreta jhlegarreta added the type:template and tools Issue about template and tools label Mar 29, 2021
@netlify
Copy link

netlify bot commented Mar 29, 2021

Deploy preview for carpentries-dmri ready!

Built with commit 255aba1

https://deploy-preview-112--carpentries-dmri.netlify.app

@kaitj
Copy link
Collaborator

kaitj commented Mar 30, 2021

Like the idea of the 404 page, but seems like it isn't loading the Carpentries template (at least in the netlify builds). I am guessing it is suppose to mirror parts of the theme, but with the 404 message.

image

@jhlegarreta
Copy link
Contributor Author

Yes, looks like it's missing some parts of the theme. Will have a look as time permits.

@@ -0,0 +1,25 @@
---
layout: base
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
layout: base
layout: page

---
layout: base
root: .
permalink: 404.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
permalink: 404.html
permalink: /404.html

Copy link
Contributor

@josephmje josephmje left a comment

Choose a reason for hiding this comment

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

just left a few suggestions. not sure whether they will change the rendering or not.

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Apr 3, 2021

Thanks for the review @josephmje .

I tried building the site locally and the 404 HTML takes the theme correctly, so maybe it's some netlify artifact.

404_dmri_carpentries

Copy link
Collaborator

@kaitj kaitj left a comment

Choose a reason for hiding this comment

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

I am good with merging this in spite of Netlify not rendering. Did a cross-reference with the 404 page in the SDC-BIDS-IntroMRI repo and everything is the same.

I suggest taking a quick look following merge to make sure the published page renders correctly

@jhlegarreta jhlegarreta merged commit b8bc782 into carpentries-incubator:master Apr 3, 2021
@jhlegarreta jhlegarreta deleted the Bring404PageFromLessonExample branch April 3, 2021 12:54
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Apr 3, 2021

We went ahead and merged this hypothesizing that it was maybe a netlify artifact what is shown in the picture here.

It turns out that netlify was correct, e.g.
https://carpentries-incubator.github.io/SDC-BIDS-dMRI/diffusion_tensor_imaging2/index.html

I've only been able to find a single relevant commit to the 404 page in the examples repository:
carpentries/lesson-example@d8f93f0

And we've realized that the example lesson does also suffer from the same issue:
https://carpentries.github.io/lesson-example/04-formatting2/index.html

Looks like we are missing something elsewhere. @tobyhodges @zkamvar do you happen to have an intuition of what could be missing?

We've realized that it's only happening on lesson pages; if you have enter an invalid URL on the main page, it seems to render correctly, e.g.
https://carpentries-incubator.github.io/SDC-BIDS-dMRI/test

Cross-referencing carpentries/lesson-example#335.

Thanks.

@jhlegarreta
Copy link
Contributor Author

Cross-referencing #140.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:template and tools Issue about template and tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants