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

Deprecate wagtailadmin/shared/header.html template #10148

Open
lb- opened this issue Feb 24, 2023 · 17 comments
Open

Deprecate wagtailadmin/shared/header.html template #10148

lb- opened this issue Feb 24, 2023 · 17 comments
Assignees
Labels

Comments

@lb-
Copy link
Member

lb- commented Feb 24, 2023

Is your proposal related to a problem?

To ensure that the header template classes are better aligned with long term classes and to finish off some of the great header styling unification work it would be good to tidy up a few remaining items with the header.

Describe the solution you'd like

  • Remove right_column_classname block usage (keep the classes) - this is should be marked as no longer supported in the release notes.
  • Replace .row class with w-header__main
  • Replace .left class with w-header__primary
  • Replace .right class with w-header__secondary
  • Align the styles in client/scss/components/_header.scss and client/scss/components/_modals.scss to use the new classes
  • client/scss/components/_header.scss should be updated to use flexbox and avoid floats to have the exact same layout as current state, floats cause issues with RTL support and now that we no longer support IE11 we should align with newer approaches
  • Important: Ensure that the snippet header usage has the same class name changes - see wagtail/snippets/templates/wagtailsnippets/snippets/type_index.html
  • Update any release notes with the class name changes

Describe alternatives you've considered

  • Leave as is, this is not the most critical change but it would be good for this shared template

Additional context

Testing this is really critical, especially cross browser and across all the variations of header (including some modal headers) when doing this.

It appears that some header parts use .col on the left/right containers - we may need to add a root class modifier to make this styling easier or see if these were actually even needed in the first place.

See related planned and previous work with these areas of styles and shared header usage

Will need to raise a new issue after this - the clean up step above and probably should try to align the right/left class names with BEM naming if possible.
Originally posted by @lb- in #8907 (comment)

@harivamsi9
Copy link

Hi, I am interested in taking this issue, Can you please assign this to me?

@Vijaykv5
Copy link

Hey , May You assign this issue to me

@lb-
Copy link
Member Author

lb- commented Feb 24, 2023

@harivamsi9 @Vijaykv5 please take the time wo walk through the WIP guide for first time contributors here. #10070

Happy for you @harivamsi9 to give this a go but please make sure that you're ready to start and have taken the time to get your Dev environment runningz understand the issue, where the code is used, how the CSS is written etc.

@harivamsi9
Copy link

thanks @lb-. Will Fully go through the required documentation, and will reach out for any questions.
Thanks for the opportunity.

@monmon2003
Copy link

Hi, can you assign this issue to me.

@Ananjan-R
Copy link

I would also like to attempt this issue

@HimanshuGarg47
Copy link
Contributor

Is there anything that I can do, Please let me know

@GeekGawd
Copy link
Contributor

Hi! @monmon2003 Issues are only assigned to the core team members of Wagtail... You can feel free to work on the issue... but @harivamsi9 and @Vijaykv5 already seems to be working on the issue, so I think your effort will be well worth it to work on some other issue...

@kshitiz305
Copy link

@lb- Hey Ben, I am new to this project and would like to know if there is any python related issue.

@lb-
Copy link
Member Author

lb- commented Apr 11, 2023

@kshitiz305 please read our new contributors guide here. #10070

Best to reach out in the slack channel for new contributors if you are looking for something specific to work on.

@Ananjan-R
Copy link

Hi, I would like to work on the issue as it seems that there haven't been any contributions made regarding the issue as of recent.

@s4shantanu
Copy link

Hii,
This issue is resolved or still open?

@lb-
Copy link
Member Author

lb- commented Apr 18, 2023

Anyone is free to work on things, see our contributor guide - #10070

There is no need to 'claim' something, if you are working on it, create a PR or a comment and go for it.

There was a draft PR created but it has been nearly two months since the review with no response, so I think it's fine for anyone else to pick this up or start from scratch without worrying about doubling up effort. #10152

We just ask that you ensure you actually have a development environment running first, even if it means you use this issue as a thing to test out changes with. Sometimes it can take a while to get that set up done, so be sure you get over that hurdle first, all of this is covered in the guide.

@Ananjan-R @s4shantanu

@s4shantanu
Copy link

Thanks @lb- for the information.

@lb-
Copy link
Member Author

lb- commented Jul 1, 2023

Removing good first issue label as this seems to have had a few attempts that have struggled with this.

Anyone is still fine to put up a PR though.

Some solid progress was made on #10152 - see comments for feedback on what needed improvement.

@laymonage laymonage self-assigned this Jul 4, 2023
laymonage added a commit to laymonage/wagtail that referenced this issue Jul 4, 2023
@laymonage laymonage removed their assignment Jul 4, 2023
@laymonage
Copy link
Member

I stumbled into this while working on #10626. I initially thought about having a go at this, but then I remembered that the upcoming Universal Listings work (#10446) might make this issue irrelevant. Not sure if it's worth spending more time into this, so I did a workaround instead.

laymonage added a commit to laymonage/wagtail that referenced this issue Jul 10, 2023
laymonage added a commit to laymonage/wagtail that referenced this issue Jul 10, 2023
laymonage added a commit to laymonage/wagtail that referenced this issue Jul 12, 2023
@thibaudcolas thibaudcolas self-assigned this Nov 13, 2023
@laymonage laymonage changed the title Clean up row/left/right classes on Shared header template Deprecate wagtailadmin/shared/header.html template Jan 8, 2024
@laymonage
Copy link
Member

Having spent a lot of time around headers in #11332, I think we shouldn't bother spending more time into this. With most of the functionalities in wagtailadmin/shared/header.html are now available in wagtailadmin/shared/headers/slim_header.html and we want to apply the slim header (with breadcrumbs) everywhere in the future, it's better to focus our efforts in adopting slim_header.html more widely across the admin. When all views have adopted the new header, we can deprecate wagtailadmin/shared/header.html (or replace it with slim_header.html).

I've updated the issue title to reflect this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Status: No status