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

Move migration guides #605

Merged
merged 11 commits into from Mar 14, 2023
Merged

Conversation

akappel
Copy link
Contributor

@akappel akappel commented Mar 13, 2023

Description

Moves Migration Guides out of the Book to their own section under Learn.

Implementation

There's a new folder under content/learn: content/learn/migration-guides. Accompanying this is a new migration-guide-section.html and some other changes to get everything working. I tried to rely on the already created book macros file. Hopefully this doesn't get confusing.

I also created a really basic SVG for the Learn page. Never done it before so if someone with more skill wants to create a better one, feel free!

Screenshots

Screen Shot 2023-03-13 at 12 35 32 AM

Screen Shot 2023-03-13 at 12 35 50 AM

@alice-i-cecile
Copy link
Member

The kerning in that 0.X - 0.Y image is a bit rough ;p I really like the image otherwise!

@akappel
Copy link
Contributor Author

akappel commented Mar 13, 2023

@alice-i-cecile I figured it'd be a placeholder until the SVG heavyweights can make something pretty 😅 didn't wanna reuse the other SVGs.

templates/learn.html Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
+++
title = "Faq"
title = "FAQ"
Copy link
Member

Choose a reason for hiding this comment

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

sneaky to include in this pr, but I'm on board😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, I hope no one else minds. I thought it was some sort of formatting issue, but figured all caps made sense.

@@ -1,9 +1,9 @@
+++
title = "0.4 to 0.5"
weight = 1
weight = 2
Copy link
Member

@ickk ickk Mar 13, 2023

Choose a reason for hiding this comment

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

My preference would actually be to reverse the order of these sections so that the most recent migration guide appears at the top - though maybe it requires another issue.

Not sure if there's a way to tell zola to automatically reverse the weights or if it's going to require shuffling these numbers around every release.. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! I'd actually thought about that but completely forgot.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should ask @alice-i-cecile, @cart for their opinions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seconded. I already pushed the commit reversing the order, but would love to hear their thoughts. You're right that it would be a manual process every release, but not too much hassle. I was hoping that zola/tera would have a reversing mechanism; it seems like it does in some sense, but I'd have to dig a little more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into this once and I don't think it's possible outside of the manual weight-shuffling. I do agree that it's probably worth it though.

content/learn/migration-guides/_index.md Outdated Show resolved Hide resolved
<text
xml:space="preserve"
id="text111"
style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:122px;font-family:'Fira Sans ExtraBold';-inkscape-font-specification:'Fira Sans ExtraBold, ';white-space:pre;shape-inside:url(#rect113);display:inline;fill:#999999"
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the kerning of the text not being perfect for this PR, it's an easy first issue for someone else and should not block this.

That said, this text should be exported as a path in inkscape for this PR. At the moment it requires that the end-user has the (non-standard) font available to display correctly. I don't have this font on the system I checked the PR with, so it looks like this instead:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I would see how Fira Mono looks as well, to sidestep the kerning issue, and because we use the monospaced version in other "svgs of code" on the site.

If it gets too wide, maybe a diff-like format could work too?

- 0.X
+ 0.Y

I like the concept and think you should keep rolling with it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great call outs! I'll give this another pass. I like the diff format, will give that a shot. Will also see what Mono looks like to get rid of the kerning.

<script src="/optional-helpers.js"></script>
{% endblock %}

{% block page_name %}Migration Guide{% endblock %}
Copy link
Contributor

@doup doup Mar 13, 2023

Choose a reason for hiding this comment

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

The diff between book-section and migration-guide-section is this:

image

I think this can be moved to a new layout:

  • create a templates/layouts/docs.html (or whatever name fits better)
  • make both templates/book-section.html and templates/migration-guide-section.html extend the new layout
  • To pass the path var you can do: Passing a variable into the parent template Keats/tera#692 (comment)
  • I think page_name can be set by a "gran-children" 🤞 Otherwise, you could add a docs_title var and pass it with the path.

New book-section.html/migration-guide-section.html would look something like:

{% extends "layouts/docs.html" %}

{% block page_name %}Book{% endblock %}

{% block docs_vars %}
    {% set docs_root_path = "learn/book/_index.md" %}
{% endblock %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beautiful, thanks @doup. I knew there was a lot of shared functionality but I was having a hard time understanding the zola functions. I'll take a look at incorporating your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doup Unfortunately, I don't think this route works (unless I'm misunderstanding, which may very well be the case). This is what I have in my layout/templates/docs.html:

...
{% block page_name %}Docs{% endblock %}

{% block docs_vars %}
    {% set docs_root_path = "" %}
{% endblock %}

{% block mobile_page_menu %}
    {{book_macros::book_menu(prefix="mobile-menu", root=get_section(path=docs_root_path))}}
{% endblock %}

{% block page_menu %}
    {{book_macros::book_menu(prefix="page-menu", root=get_section(path=docs_root_path))}}
{% endblock %}

...

But it gives:

Reloading only template
Error: Failed to build the site
Error: Failed to render section '/Users/kap/dev/bevy-website/content/learn/book/getting-started/setup/_index.md'
Error: Reason: Failed to render 'book-section.html' (error happened in a parent template)
Error: Reason: Function call 'get_section' failed
Error: Reason: Variable `docs_root_path` not found in context while rendering 'book-section.html'
Done in 1.0s.

Which I guess makes sense because the docs_root_path is in a different "block context". I've tried path=docs_vars.docs_root_path, but no luck. I've also tried path={{docs_root_path}}, which gives a parsing error. set_global also doesn't work. I'm not sure how to share variables between blocks and the docs don't showcase that feature. I'm still trying to wrap my head around the templating language and what's available, but right now I'm having no luck.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here: akappel#1

The thing is that as the error says the block must be defined before the vars are being used; the solution in this case was to move the block definition to the top of the root layout (base.html). This way it works.

I've changed the block name to extra_data as suggested in the linked issue, this way is more generic and makes sense for any layout that extends base.

@akappel
Copy link
Contributor Author

akappel commented Mar 14, 2023

@ickk @rparrett: The SVG has been updated to Fira Mono and pathed:
Screen Shot 2023-03-14 at 3 44 04 PM
Thoughts?

@doup @alice-i-cecile @cart: I've consolidated book-section.html and migration-guide-section.html into a unified docs.html (name pending). It uses section.components plus some filters to accurately get the root_section_path (as long as it's two folders deep). I know this significantly increases the size of this PR, but it should be able to be extended to the quick-start guide/new-book/migration-guides separation in the future.

@akappel akappel requested review from doup, rparrett and ickk and removed request for IceSentry, mockersf, doup and rparrett March 14, 2023 19:49
@IceSentry
Copy link
Contributor

Would it be possible to add a red/green accent to the -/+? Just to look a bit more like a diff

@akappel
Copy link
Contributor Author

akappel commented Mar 14, 2023

@IceSentry I thought about that, and the only reason I didn't was because the "Learn" page is pretty monochrome, and I didn't wanna splash color if that was an aesthetic @cart wanted to keep. So I opted for a "highlighted greyscale" look.

Edit: I can extend that highlighting to the -/+ as well! Or colorize if everyone's okay with that.

@alice-i-cecile
Copy link
Member

migration-guides

Feel free to steal this edit :) I'm sad about the kerning still, but the alignment is too nice to give up

@akappel
Copy link
Contributor Author

akappel commented Mar 14, 2023

@IceSentry @alice-i-cecile 💥 :
Screen Shot 2023-03-14 at 4 23 46 PM

@IceSentry
Copy link
Contributor

That's why I suggested only the symbols to keep the monochrome, but I like having some color in there personally

@rparrett
Copy link
Contributor

rparrett commented Mar 14, 2023

Some feedback specifically on the svg: I think we should try the bold or medium variant of the font, and I agree that colorizing the symbols only (still potentially using shades of grey elsewhere) might be best.

But the main reason I'm not currently digging the colors is that the actual colors are pretty far from the level of saturation/lightness of the color scheme used in other "code svgs" on the site.

edit: line spacing seems worth adjusting (lower) too.

@rparrett
Copy link
Contributor

image

migration-paths.svg.zip

The only thing I'm not thrilled about is that they end up a bit wider than other icons at the same height, because the aspect ratio isn't square. But I've already done as much horizontal squishing and line-height-increasing that I felt like I could get away with.

@akappel
Copy link
Contributor Author

akappel commented Mar 14, 2023

@rparrett I've gone ahead and updated to your SVG. I think it hits the right balance of what everyone is looking for.

Copy link
Member

@ickk ickk left a comment

Choose a reason for hiding this comment

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

LGTM

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review Ready for a maintainer to consider for merging label Mar 14, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 14, 2023
Merged via the queue into bevyengine:main with commit 3143eac Mar 14, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Book C-Webdev S-Ready-For-Final-Review Ready for a maintainer to consider for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants