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

adopt shared header template on the dashboard view #8781

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

PaarthAgarwal
Copy link
Member

@PaarthAgarwal PaarthAgarwal commented Jun 30, 2022

Addresses #8539.
Applied shared header to dashboard.
Before:

Screenshot from 2022-06-30 15-57-54

After:

Screenshot from 2022-07-04 18-29-46

Screenshot from 2022-07-04 18-31-07

Screenshot from 2022-07-04 18-31-35

Edit1: Updated Screenshot
Edit2: Updated Screenshot

@squash-labs
Copy link

squash-labs bot commented Jun 30, 2022

Manage this branch in Squash

Test this branch here: https://paarthagarwalhome-header-1s55u.squash.io

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

@PaarthAgarwal great start on the way here, I would like to see the styling get a bit closer to the current state though.

I have put specific code feedback in the review comments, but also some general feedback

  • The PR title could be a bit clearer 'use/adopt shared header template on the dashboard view'
  • When the shared header template changes are closer, can you also include a storybook (pattern library) variant that has the avatar in it, this will really help for testing later

Great start here!

wagtail/admin/templates/wagtailadmin/shared/header.html Outdated Show resolved Hide resolved
wagtail/admin/templates/wagtailadmin/shared/header.html Outdated Show resolved Hide resolved
wagtail/admin/templates/wagtailadmin/shared/header.html Outdated Show resolved Hide resolved
wagtail/admin/templates/wagtailadmin/shared/header.html Outdated Show resolved Hide resolved
wagtail/admin/templates/wagtailadmin/home.html Outdated Show resolved Hide resolved
client/scss/layouts/_home.scss Outdated Show resolved Hide resolved
wagtail/admin/templates/wagtailadmin/shared/header.html Outdated Show resolved Hide resolved
@lb-
Copy link
Member

lb- commented Jun 30, 2022

With the styling - it may need some careful work but it looks like there is a bit of an over use of position absolute here and this relates to I think the styles trying to work around the sidebar.

On top of that the existing styles are a mix of tailwind apply and normal scss so it is a bit of a minefield.

So far I have found that you can change the following in client/scss/components/_header.scss...

  • h1 { - get rid of the position relative
  • > svg.icon { - get rid of the position absolute and the inset-inline-start: -1.5em;
  • This makes the icons appear roughly the same and the avatar appear the same also (yay)
  • But everything is a bit far 'in'
  • This comes back to the padding-inline-start: 110px; which is a work around for the sidebar on mobile (note different breadpoints will have different padding)
  • But ... maybe this gives you a start of where to look
  • We can check with Thibaud but we should probably unravel all the apply and use the theme approach instead - we can chat more about this on our next catch up.

@lb-
Copy link
Member

lb- commented Jul 2, 2022

@PaarthAgarwal Can you please do a seperate branch / PR for the dashboard stats design updates.

It will.be easier to review in isolation.

Plus - we may end up getting the stats thing done much easier due to the discussion about header component stuff that is ongoing.

@PaarthAgarwal
Copy link
Member Author

@PaarthAgarwal Can you please do a seperate branch / PR for the dashboard stats design updates.

It will.be easier to review in isolation.

Plus - we may end up getting the stats thing done much easier due to the discussion about header component stuff that is ongoing.

Sure

@PaarthAgarwal PaarthAgarwal changed the title added shared header to dashboard adopt shared header template on the dashboard view Jul 2, 2022
@lb-
Copy link
Member

lb- commented Jul 4, 2022

@PaarthAgarwal a few other things

  • We can probably remove header--home styles as not used now
  • Might be worth adding the prefix of w-header to the root header class + w-header__title etc (but leave left/col/row as is)

@PaarthAgarwal
Copy link
Member Author

  • w-header
  1. Removed the header--home styles.
  2. Added the prefix w- to header classes in HTML, but it'll be better to put those changes in a separate PR because it may open a can of worms with failing tests and styling issues.
  3. Updated screenshots in the first comment.

@PaarthAgarwal PaarthAgarwal marked this pull request as ready for review July 4, 2022 19:16
@lb- lb- self-requested a review July 5, 2022 04:40
@lb-
Copy link
Member

lb- commented Jul 5, 2022

good call on the classes @PaarthAgarwal thanks.

client/scss/layouts/_home.scss Outdated Show resolved Hide resolved
@PaarthAgarwal
Copy link
Member Author

@PaarthAgarwal a few other things

  • We can probably remove header--home styles as not used now
  • Might be worth adding the prefix of w-header to the root header class + w-header__title etc (but leave left/col/row as is)

Interestingly there were no styles for .header classes nor any test for them. All the header styles are written for the header element directly.

{% load i18n %}
{% block branding_welcome %}
{% blocktrans trimmed %}Welcome to the {{ site_name }} Wagtail CMS{% endblocktrans %}
{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

might be good to add a new line here and also configure your editor to add line breaks at the end of your file.

@@ -153,6 +153,12 @@ def page_permissions(context, page):
return _get_user_page_permissions(context).for_page(page)


@register.simple_tag
def classnames(*classes):
# classes # will be an array
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# classes # will be an array
"""
Returns any args as a space-separated joined string for using in HTML class names.
"""

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

Code is looking pretty solid.
Now need to do a review of the UI changes in a few browsers.

  • would be good to add a bit more margin after the avatar or pull the avatar over to the left a bit more.

@lb-
Copy link
Member

lb- commented Jul 5, 2022

Update - there may be a refined design for the home (dashboard) view coming in the next week or so so this can probably come in mostly as is.


```html+django
{% extends "wagtailadmin/home.html" %}
To replace the welcome message on the admin home page (dashboard), create a template file `dashboard/templates/wagtailadmin/home/branding_welcome_title.html` that overrides the content:
Copy link
Member

Choose a reason for hiding this comment

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

Note that if we went ahead with an approach like #8802, we wouldn’t need to refactor this as much.

@lb-
Copy link
Member

lb- commented Jul 6, 2022

@PaarthAgarwal I have done some tweaks to this branch and will merge in shortly - basically applying what we discussed, but also including Thibaud's template approach #8802

.homepage {
header {
.header__title {
inset-inline-start: -1.5em;
Copy link
Member

Choose a reason for hiding this comment

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

To simplify things I opted for a shared header styles approach only, see other comment below.

{% block breadcrumb %}{% endblock %}
<div class="row">
<div class="left">
<div class="col">
<h1 class="header__title">
{% if icon %}{% icon name=icon %}{% endif %}
{% if icon %}
{% icon class_name="w-header__icon" name=icon %}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{% icon class_name="w-header__icon" name=icon %}
{% icon class_name="w-header__glyph" name=icon %}

Having a generic class for the icon and avatar - we can make styles a bit easier to follow.

Copy link
Member Author

Choose a reason for hiding this comment

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

That'll be great!

{% if icon %}
{% icon class_name="w-header__icon" name=icon %}
{% elif avatar %}
<div class="avatar"><img src="{{ avatar }}" alt="" /></div>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div class="avatar"><img src="{{ avatar }}" alt="" /></div>
<div class="w-header__glpyh avatar"><img src="{{ avatar }}" alt="" /></div>

Similar to above.

- use classnames template tag in shared header template
- add classname as documented variable for the shared header template
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

4 participants