-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Implement new designs for header/breadcrumbs in slim_header.html #11332
Conversation
Manage this branch in SquashTest this branch here: https://laymonageheaders-new-design-04on0.squash.io |
417c028
to
9fea602
Compare
1bb653e
to
3fac354
Compare
3330e57
to
bc9d16c
Compare
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.
@laymonage looking pretty solid, could you take some time to move more of your TODO comments to a GitHub issue or similar? They seem valuable to track future progress but not critical enough for us to get to on short notice.
I’ll review this later in more depth.
736fc75
to
dadc272
Compare
@@ -1,4 +1,4 @@ | |||
.page-explorer .w-breadcrumbs { | |||
.w-breadcrumbs:not(.editor-view .w-breadcrumbs) { |
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.
From commit message:
We want to make the slim_header.html as the basis for the modern header
template that will be used everywhere, eventually replacing header.html.
Rather than making the page explorer be the special case where we want
the last item to be bigger, do it the other way around i.e. use the
smaller font size on the create/edit views. This ensures that the big
font size is used on all other views e.g. Inspect view.
@@ -2,7 +2,7 @@ | |||
{% load i18n wagtailadmin_tags %} | |||
{% block titletag %}{% blocktrans trimmed with snippet_type_name=model_opts.verbose_name %}New {{ snippet_type_name }}{% endblocktrans %}{% endblock %} | |||
{% block content %} | |||
{% include 'wagtailadmin/shared/headers/slim_header.html' %} | |||
{% include 'wagtailadmin/shared/headers/slim_header.html' with breadcrumbs_items=breadcrumbs_items side_panels=side_panels history_url=history_url %} |
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.
We can't use only
yet for snippets and pages due to #11358
@@ -7,9 +8,34 @@ | |||
{% block header %} | |||
{% block slim_header %} | |||
{% if breadcrumbs_items %} | |||
{% fragment stripped as actions %} |
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.
Use the stripped
option so that if both blocks are empty, the resulting fragment is actually empty instead of being a string consisting of only whitespaces. This ensures that when the fragment is passed down to the slim_header.html
, you can check its emptiness with if
(horrible, I know, but give me something better?)
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.
Indeed pretty horrible! This doesn’t strike me as a dealbreaker but definitely one for a list of refactorings.
@@ -69,6 +69,8 @@ def get_url(self, url_name, args=()): | |||
|
|||
|
|||
class TestCustomIcon(BaseSnippetViewSetTests): | |||
# TODO: decide what to do with this test, since the new designs after |
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.
To be addressed with #11366
@@ -156,7 +155,6 @@ def get_template_names(self): | |||
|
|||
class IndexView(generic.IndexViewOptionalFeaturesMixin, generic.IndexView): | |||
view_name = "list" | |||
table_class = InlineActionsTable |
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.
This is a leftover I forgot to cleanup in #11295
{% if not search_url %} | ||
{% elif search_form or filters %} |
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.
Empty if, to avoid extra indent
The <form>
now works with search_form
or filters
(or both). You can have filters without search and vice versa, but use the same URL in general (the index results URL)
@@ -858,6 +857,7 @@ def workflow_history_detail_view(self): | |||
object_icon=self.icon, | |||
header_icon="list-ul", | |||
workflow_history_url_name=self.get_url_name("workflow_history"), | |||
_show_breadcrumbs=False, |
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.
See https://static-wagtail-v5-2.netlify.app/admin/snippets/base/person/workflow_history/4/detail/3/ for why I added this, it's not ready to use breadcrumbs yet and I should've done this in 5.2
page_title and page_subtitle are not meant to be read as one phrase
…e_buttons To follow the pattern for get_list_buttons and get_list_more_buttons This will be made clearer in the next commit
And rename get_results_url() in pages' BaseIndexView to get_index_results_url to match the method in generic IndexView
10px was never the right width. Using CSS instead of the column's width attribute allows us to use media queries to change the width responsively
…tables The added CSS handles the case when nice padding is removed (to make the listing go full-width) and no bulk actions are available (e.g. a ModelViewSet index view or some other view that extends generic listing view). This allows us to not rely on the full-width class (which should eventually be removed) while correctly apply the spacing regardless whether nice-padding is used or not, and whether bulk actions are available or not. If nice-padding is used, then the listing view will look as they previously do.
Report pages have styles for .report that adds margins similar to nice-padding
…form Also make the search form optional. The <form> element may still be rendered as if there is either the search form or the filters, or both.
Before c8edfd1, empty q is considered invalid, so is_searching is False. After that commit, empty q is valid so set is_searching based on the truthiness of the search_query instead
…_results.html template Missed this in 4082acc
Even though they have tooltips on hover/focus, axe will complain if they don't have aria-label
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 @laymonage!
I noted an issue with the snippets’ "mode index" view which was displaying an "Add" button when it’s not meant to, so took the liberty to override header_buttons
there too.
Other than that, follow-up changes needed:
- Locale selector,
wagtail/admin/templates/wagtailadmin/generic/form.html
, possibly elsewhere - Copy-pasted w-header,
wagtail/admin/templates/wagtailadmin/generic/form.html
- TODO comments,
wagtail/snippets/tests/test_viewset.py
- TODO comment,
wagtail/admin/views/pages/revisions.py
- A better approach to snippets
Tested in Chrome 120, Firefox 121, Firefox 115 ESR
|
||
input[type='checkbox'] { | ||
margin-inline-end: 0; | ||
&:has(.bulk-action-checkbox) { |
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.
:has browser support isn’t good enough for us to use it for any critical styles. In this case I’ve confirmed the only issue would be the checkboxes being placed too far to the left so this seems acceptable.
@@ -1091,6 +1091,7 @@ def register_icons(icons): | |||
"rotate.svg", | |||
"search.svg", | |||
"site.svg", | |||
"sliders.svg", |
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.
This will also need adding to our wagtail_icons_table.txt
when merging. I can take care of that.
<div id="listing-results"> | ||
{% if breadcrumbs_items and locale %} | ||
{# TODO: temporarily put the locale selector here since we no longer use the legacy header #} |
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.
Yes this needs adjusting. Seems reasonable enough to me as long as we do complete this in time for the release.
@@ -7,9 +8,34 @@ | |||
{% block header %} | |||
{% block slim_header %} | |||
{% if breadcrumbs_items %} | |||
{% fragment stripped as actions %} |
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.
Indeed pretty horrible! This doesn’t strike me as a dealbreaker but definitely one for a list of refactorings.
dadc272
to
d866d7d
Compare
d866d7d
to
571c198
Compare
I think this PR may have introduced an issue - see #11704 |
Old description
WIP
The snippets views still look a bit funny atm as they still use both
slim_header.html
andheader.html
. Will work on porting features over fromheader.html
toslim_header.html
and remove/reduceheader.html
usage.Please check the following:
make lint
from the Wagtail root.Please describe additional details for testing this change.
Footnotes
Development Testing ↩
Browser and device support ↩
Accessibility Target ↩