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

[WD-17951] use different footer for docs pages #1474

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akbarkz
Copy link
Contributor

@akbarkz akbarkz commented Dec 20, 2024

Done

Moved a footer that was implemented for multipass.run's Docs pages some time ago into canonical.com and used it for all Docs pages on canonical.com.

QA

  • Open demo instance
  • Open Docs pages, for example at /multipass/docs or /mircostack/docs and check that they use their own footer
  • Open other pages (not Docs) and check that they still use an existing footer (similar to production)

Issue / Card

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

Screenshots

Normal footer:
Screenshot From 2024-12-20 11-18-52

Docs footer:
Screenshot From 2024-12-20 11-19-16

@webteam-app
Copy link

Copy link
Contributor

@petesfrench petesfrench left a comment

Choose a reason for hiding this comment

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

The l-docs__footer for use with the l-docs pattern, which these docs are not currently using. You should either use l-footer or better yet, use l-docs on the whole page

href="https://www.ubuntu.com/legal">Legal information</a>
</li>
<li class="p-inline-list__item">
<a class="js-revoke-cookie-manager" href="">Manage your tracker settings</a>
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
<a class="js-revoke-cookie-manager" href="">Manage your tracker settings</a>
<a class="js-revoke-cookie-manager" href="#">Manage your tracker settings</a>

If JS is disabled this will refresh the page, it's better that it just does nothing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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 suppose multipass was also wrong

<div class="row">
<footer class="l-docs__subgrid">
<div class="l-docs__sidebar">
<p style="padding-left: 1.5rem">
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
<p style="padding-left: 1.5rem">
<p>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is the full-width site example. The one used here https://microk8s.io/docs

Copy link
Contributor

Choose a reason for hiding this comment

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

It also breaks on medium screens
image

@akbarkz
Copy link
Contributor Author

akbarkz commented Dec 20, 2024

The l-docs__footer for use with the l-docs pattern, which these docs are not currently using. You should either use l-footer or better yet, use l-docs on the whole page

I think wrapping the whole docs page with "l-docs" class would be tricky, considering it currently extends base_index.html. How about I wrap only the docs footer?

@petesfrench
Copy link
Contributor

@akbarkz We managed to do this in the other micro sites, it just required some extra work in jinja, out of scope for this piece of work but still needs to be done.

I still don't think you should use the l-docs__footer here as it this is not how it is supposed to be used and has potential to break

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.

3 participants