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

Upgrade to vanilla-4.5.0 & implement new documentation layout #510

Merged
merged 11 commits into from
Jan 29, 2024

Conversation

petesfrench
Copy link
Contributor

@petesfrench petesfrench commented Jan 15, 2024

Done

  • Upgrade to vanilla-4.5.0
  • Implement new documentation layout

QA

  • Compare to https://microk8s.io/docs as this is live and approved by design
  • Check code, different screen sizes, etc
  • Check that none of the SCSS i removed breaks anything
  • Check different pages, search results etc

Issue / Card

Fixes https://warthogs.atlassian.net/browse/WD-8265

@webteam-app
Copy link

Demo starting at https://juju-is-510.demos.haus

@petesfrench
Copy link
Contributor Author

@lyubomir-popov Do you think we need to do anything different with the footer on docs pages?

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2d35d49) 58.51% compared to head (cfedde2) 58.51%.
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #510   +/-   ##
=======================================
  Coverage   58.51%   58.51%           
=======================================
  Files           6        6           
  Lines         188      188           
=======================================
  Hits          110      110           
  Misses         78       78           
Flag Coverage Δ
python 58.51% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bartaz
Copy link
Contributor

bartaz commented Jan 16, 2024

Probably not directly related to this PR, but rather docs content or discourse, or discourse module, but most of the pages don't seem to have automatic table of contents.

Docs home page does (Contents list on the right): https://juju-is-510.demos.haus/docs/juju
But others don't, even when they have H2s in content: https://juju-is-510.demos.haus/docs/juju/upgrade-your-juju-deployment-from-2-9-to-3-x

So either on Discourse side something is not rendering as expected, or discourse module is not parsing them.

static/sass/styles.scss Outdated Show resolved Hide resolved
@bartaz
Copy link
Contributor

bartaz commented Jan 16, 2024

Footer is misaligned in docs layout:

image

If we have such footer that doesn't split into sidebar/main grid, as the default one, we should keep the whole content footer aligned with main part of the docs layout, so it still needs to be put inside subgrid (but only in documentation layout, not in brochure site layout).

Something like:

<div class="p-strip--dark footer l-docs__subgrid">
   <div class="l-docs__main">
       // footer contents
   </div>
</div>

@petesfrench
Copy link
Contributor Author

petesfrench commented Jan 17, 2024

Probably not directly related to this PR, but rather docs content or discourse, or discourse module, but most of the pages don't seem to have automatic table of contents.

Docs home page does (Contents list on the right): https://juju-is-510.demos.haus/docs/juju
But others don't, even when they have H2s in content: https://juju-is-510.demos.haus/docs/juju/upgrade-your-juju-deployment-from-2-9-to-3-x

So either on Discourse side something is not rendering as expected, or discourse module is not parsing them.
I believe this is happening somewhere on the discourse side. The headings_map list we use is created by discourse as part of the update, and on the pages we are not receiving anything. Not sure exactly why, I will investigate. But I don't think this should be a blocker for this reason.

@petesfrench
Copy link
Contributor Author

@bartaz Are you able to take another look please?

@bartaz
Copy link
Contributor

bartaz commented Jan 17, 2024

Footer padding seems to be doubled in docs layout (I think the dark strip is nested in another dark strip?)

image

@@ -285,3 +287,12 @@ html {
.github-buttons {
margin-bottom: $sp-medium;
}

// TODO: to be removed when properly fixed on Vanilla side
// https://github.com/canonical/vanilla-framework/issues/4898
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm working on a fix for canonical/vanilla-framework#4898 this Pulse, so maybe we can drop this workaround already?

This will mean top menu will be a bit misaligned for short time until fix is released and updated here, but at least the workaround will not spread around unnecessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine with this. Is there anything I can do to help those changes getting merged ASAP?

@@ -75,6 +72,11 @@ $breakpoint-navigation-threshold: 900px;
}
}

body:not(.docs) .p-navigation__banner {
Copy link
Contributor

Choose a reason for hiding this comment

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

I opened an issue to address that on Vanilla side, please add a comment for reference:
canonical/vanilla-framework#4947

Suggested change
body:not(.docs) .p-navigation__banner {
// TODO: to be removed when fixed in Vanilla
// https://github.com/canonical/vanilla-framework/issues/4947
body:not(.docs) .p-navigation__banner {

@petesfrench
Copy link
Contributor Author

Hey @TeodorPt, can you take a look at https://juju-is-510.demos.haus/docs and make sure everything looks OK with the new documentation layout?

@Lukewh Lukewh force-pushed the update-documentation-layout branch from 5e56093 to 2789849 Compare January 22, 2024 10:41
@petesfrench
Copy link
Contributor Author

@bartaz could you give this another review please? I have updated with your suggestions

@bartaz
Copy link
Contributor

bartaz commented Jan 22, 2024

It seems that in the meantime a PR was merged that (in my opinion wrongly) applies 100% width to paragraphs in docs.

This should not happen, especially in new layout that uses whole 12 grid width for the content.
New documentation layout uses u-text-max-width on l-docs__main, so it should address the issue of text lists max width being mismatched with rest of the text.

So the changes in "max-width" commit should be removed/reverted if possible. I previously mentioned it in #514.

@Lukewh
Copy link
Contributor

Lukewh commented Jan 22, 2024

@bartaz is there work in the pipeline to fix this kind of visual “issue”:

image
(see also)
canonical/charmhub.io#1448

and the inconsistencies with tables:

image

I just looked through #369, and I don't believe this was ever solved, it was just left without a concrete answer for @tmihoc.

@bartaz
Copy link
Contributor

bartaz commented Jan 22, 2024

@bartaz is there work in the pipeline to fix this kind of visual “issue”:

Notifications in Vanilla don't have max-width set on content. If this happens with "notes" from Discourse, there must be something wrong with markup they produce (a discourse module bug it seems?).

https://vanillaframework.io/docs/patterns/notification

and the inconsistencies with tables:

Not sure what is the issue with tables? Tables, as blocks are meant to be full width. As for the content within, I'm not sure, especially what is meant to happen if you put paragraphs in tables.

If something seems to be a bug in Vanilla opening a ticket in Vanilla repo would be the best way to kick of the discussion.

@petesfrench
Copy link
Contributor Author

@bartaz I would like to get this merged by the end of the pulse (Thursday for me), can you advise me what I need to do to get it to that point please? I have removed the custom max-width styling already.

Copy link
Contributor

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fixes.

@bartaz bartaz mentioned this pull request Jan 25, 2024
@tmihoc
Copy link
Member

tmihoc commented Jan 29, 2024

I gave it a try. Looks great. Suggestion: Would it be possible for people to get the link from somewhere near the in-page title as well? That seems to be standard behavior on, e.g., Read the Docs. For example, on this page https://ops.readthedocs.io/en/latest/#module-ops.pebble you can get a section link from the side navigation but also from the little thingie (highlighted below) next to the in-page title
image

@petesfrench petesfrench merged commit 90a6a8a into main Jan 29, 2024
8 checks passed
@petesfrench petesfrench deleted the update-documentation-layout branch January 29, 2024 12:15
@tmihoc
Copy link
Member

tmihoc commented Jan 29, 2024

@bartaz @petesfrench @Lukewh I've started converting the manually formatted titles from Juju docs to plain Markdown, so they would work well with this, but then noticed that the automatic TOC does not pick up the h3 headings. Coupled with the fact that (as I mentioned above) titles that don't show up in the automatic TOC aren't linkable, this is an issue for me -- I don't often use h3 headings but, when I do, they're necessary, and I'd like to be able to link to them. Insofar as I can see, my only solution right now is to find a manual hack (e.g., use h2 formatting with a manual twist, or something like that). This is obviously not ideal. Any plans to support h3 headings in the automatic TOC as well?

@bartaz
Copy link
Contributor

bartaz commented Jan 29, 2024

As far as I can see in the code, both h2 and h3 are supposed to be picked up: https://github.com/canonical/canonicalwebteam.discourse/blob/main/canonicalwebteam/discourse/parsers/docs.py#L734

So if h3s don't show up, it seems to be a bug in discourse module (module itself or the template), so needs to be reported on the repo https://github.com/canonical/canonicalwebteam.discourse

@tmihoc If you have an example of a page with h3s that don't show up in table of contents, I'd suggest opening an issue here: https://github.com/canonical/canonicalwebteam.discourse/issues) with a link and description what is expected.

@tmihoc
Copy link
Member

tmihoc commented Jan 30, 2024

@tmihoc If you have an example of a page with h3s that don't show up in table of contents, I'd suggest opening an issue here: https://github.com/canonical/canonicalwebteam.discourse/issues) with a link and description what is expected.

@bartaz Thanks! I filed an issue here: canonical/canonicalwebteam.discourse#187 How can I make sure it gets prioritized? As I was saying there, I will not abandon my current manual setup until this is addressed, as the automatic result is worse from the point of view of UX than what I had before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants