-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: Implement a modern footer #400
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #400 +/- ##
==========================================
+ Coverage 82.72% 83.87% +1.15%
==========================================
Files 124 129 +5
Lines 2599 2710 +111
==========================================
+ Hits 2150 2273 +123
+ Misses 449 437 -12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good :) I just have a few small comments.
[ | ||
("link", LinkBlock()), | ||
], | ||
blank=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Minor] Should we be allowing a column with no links?
[Minor] Also, if there's a maximum number of columns supported by the design, we should mention it here as help text, and add a max_num
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I'm not a fan of making body content mandatory. No content editor will create an empty column and it doesn't really break anything. It just makes testing more annoying. Also, each column is a different snippet, so there's no good way to enforce a limit. But again, the content editor can make a judgement call for the number of columns, taking into account the length of the titles and link texts, and using the preview to see what it looks like.
ietf/templates/includes/footer.html
Outdated
<a | ||
href="{{ link.value.url }}" | ||
class="link-underline-opacity-0 link-light fw-semibold" | ||
> | ||
{{ link.value.text }} | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Minor] We might want to add more space between links on mobile so that it's not as easy to tap the wrong link.
[Accessibility] This text's colour contrast fails accessibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!
show_in_menus=True, | ||
) # type: ignore | ||
|
||
def test_links(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nitpick] Please add some comments for the FE devs. :)
@SharmaineLim thanks! I'll mark the PR as "Ready for review" so that IETF have time to review it too. |
Fix #377