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

Update header to make it easier to find support, guidance and where to log in #397

Conversation

DilwoarH
Copy link
Contributor

@DilwoarH DilwoarH commented Jan 8, 2018

This is part of a larger piece of work to change the homepage styling so it is easier to find things on the page and consistent with other services in the service toolkit.

Examples of other pages that use this pattern can be found under the components heading on the service toolkit. https://www.gov.uk/service-toolkit

The code for these pages can be found here: https://github.com/alphagov/product-page-example

As part of this work we will make support easier to find and remove the beta phase banner

The guidance link would go to: https://www.gov.uk/government/collections/digital-marketplace-buyers-and-suppliers-information

The GOV.UK Digital Marketplace logo would go to the digital marketplace home page.

Ticket ID: https://trello.com/c/mityCieG/106-update-header-to-make-it-easier-to-find-support-guidance-and-where-to-log-in

screen shot 2018-01-12 at 11 15 26

<h1>
{{ heading }}
</h1>
{%- if heading_level == '2' -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

The "page heading" macro is intended to be just that - the heading for a whole page - which is why it's always a h1. This makes sense to me as a toolkit macro - "this is how our top level heading on every page is styled", and that could easily change to be more than just the h1 element it is currently.

What's the need for a macro for h2 (and possibly lower) headings? The approach until now - to add heading styles to the dmspeak class - seems OK to me?

If there is a need for more heading macros then I'd be tempted to make it a new macro - "paragraph_heading" or something, rather than mixing it up with the page heading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry - this got committed by accident, it was something I was testing out for another pull request. Has been removed.

@DilwoarH DilwoarH force-pushed the dh-106-update-header-to-make-it-easier-to-find-support-guidance-and-where-to-log-in branch 6 times, most recently from d5f9873 to d82246f Compare January 12, 2018 11:06
@DilwoarH DilwoarH changed the title [WIP] Adds in proposition header Update header to make it easier to find support, guidance and where to log in Jan 12, 2018
Copy link
Contributor

@idavidmcdonald idavidmcdonald left a comment

Choose a reason for hiding this comment

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

Does mean we can also remove the phase-banner component that exists in here and will no longer be used?

VERSION.txt Outdated
@@ -1 +1 @@
27.0.0
28.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a record to the changelog please for completeness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has been done now


.header-title {
float: left;
font-family: "nta", Arial, sans-serif;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be making use of our variables here? Above is using $toolkit-font-stack;

}

a {
color: #fff;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use $white?

@TheDoubleK
Copy link
Contributor

TheDoubleK commented Jan 15, 2018

Has this had a any product sign-off at all? Feels like too big a thing for us to just push out as "some firebreak stuff" - Tobi is on the card but hasn't said anything on there, and I don't know that she ever saw this version of the header?

I think making the guidance link more prominent should definitely be good, but I think the link text overall might be confusing - "Guidance Help Log in"... as a user I'm thinking "what's the difference between guidance and help"? Why did we decide against "Support" as I think I saw in a previous version (at least for the URL)? To me "Support" is much more clearly "I need to talk to a person" than "Help!", which could be anything.

@DilwoarH
Copy link
Contributor Author

@constancecerf - could you help answer Kev's question please?

@DilwoarH DilwoarH force-pushed the dh-106-update-header-to-make-it-easier-to-find-support-guidance-and-where-to-log-in branch from 9d281a6 to 5372dca Compare January 15, 2018 11:39
@DilwoarH
Copy link
Contributor Author

@idavidmcdonald

"Does mean we can also remove the phase-banner component that exists in here and will no longer be used?"

Yes - I believe this can now be removed.

@idavidmcdonald
Copy link
Contributor

@DilwoarH

Just so I'm clear, does that mean you will be removing the phase-banner component as part of this PR or were you just acknowledging that it can be removed?

@idavidmcdonald
Copy link
Contributor

I've been playing around in different browser window sizes and here are some screenshots for us compared to PaaS. You'll notice that unlike the other components our name and phase tag can separate when at some small screen sizes. I think some of the other components also reduce the font size slightly of the product name when at small screen sizes so things fit in nicely.

image

image

image

@DilwoarH
Copy link
Contributor Author

@idavidmcdonald - I was just acknowledging that this could be removed. I think if everyone agrees that it can be removed then I am happy to do so.

@idavidmcdonald
Copy link
Contributor

Yeah I think it can be removed and if we both agree then let's do it now.

@DilwoarH DilwoarH force-pushed the dh-106-update-header-to-make-it-easier-to-find-support-guidance-and-where-to-log-in branch 2 times, most recently from b1f2f5c to eadeb74 Compare February 1, 2018 11:27
@DilwoarH
Copy link
Contributor Author

DilwoarH commented Feb 1, 2018

@idavidmcdonald - I have updated the toolkit:

screen shot 2018-02-01 at 11 08 17

@idavidmcdonald
Copy link
Contributor

Can we remove the phase-banner component from the toolkit too please

@idavidmcdonald
Copy link
Contributor

Just had a play with it on my local - nice work on sorting the mobile resolution stuff.

I spotted one thing that a designer might care about (I don't know if it's Laurence or Jeremy working with you on this one).

image

On one of the mid sized resolutions (between desktop and mobile) you can get a header that looks like that. Should the Digital Marketplace have a slightly larger margin so it aligns horizontally with the crown logo?

Copy link
Contributor

@idavidmcdonald idavidmcdonald left a comment

Choose a reason for hiding this comment

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

Few minor comments

</li>
{% if (current_user.is_authenticated() if logged_in is not defined else logged_in) %} {% if (role or current_user.role) == 'buyer' %}
<li>
<a href="/buyers">View your account</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have external routes for /buyers and /suppliers and user/login?

### What changed

- Changes existing header to product page style header.
- Product page Github: https://github.com/alphagov/product-page-example
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have in here what changes someone would need to make to a frontend app to bring this new version of the toolkit in? As in, I assume if one of the FE apps just bumped to this toolkit version without any other changes then things would break/look wrong?

E.g. mention the changes that you are currently having to make in the buyer FE.
For example does the FE need a certain version of utils pulled in?
Removal of phase banner
Adding of inside-header block
That type of stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the instructions below

@DilwoarH
Copy link
Contributor Author

DilwoarH commented Feb 1, 2018

@idavidmcdonald - I have created the phase banner tickets for our next sprint:
https://trello.com/c/8xMrtTs0/346-fix-jenkin-job-for-frontend-toolkit-documentation
https://trello.com/c/MhnOmevf/347-remove-phase-banner

Regarding the responsiveness, I have updated it so that the padding is always right now.

header

@risicle
Copy link
Contributor

risicle commented Feb 1, 2018

For bonus points, roll this dependency out to the frontend apps one by one so that half of our pages don't match each other for a period of a few weeks.

@DilwoarH
Copy link
Contributor Author

DilwoarH commented Feb 6, 2018

@risicle - I am going to try and roll it out all in one go if possible.

@laurenceberry
Copy link

How does this look on the static pages (404 etc)?

@lauraflannery
Copy link

Hey @DilwoarH I'm happy to approve the new header.

cc @laurenceberry

In future when there are things like this being discussed in github which aren't on a team's Trello board please can you copy a product manager into the conversation so one of us has oversight? Thank you. :)

@DilwoarH DilwoarH force-pushed the dh-106-update-header-to-make-it-easier-to-find-support-guidance-and-where-to-log-in branch 3 times, most recently from 014e83e to 938a4d5 Compare February 15, 2018 10:24
@DilwoarH
Copy link
Contributor Author

@laurenceberry - this is how the 404 page looks

screen shot 2018-02-15 at 10 50 41

@DilwoarH
Copy link
Contributor Author

@idavidmcdonald - everyone has signed off the changes. Could I get this approved so I can move across everything?

Update header to make it easier to find support, guidance and where to log in

    This is part of a larger piece of work to change the homepage styling so it is easier to find things on the page and consistent with other services in the service toolkit.

    Examples of other pages that use this pattern can be found under the components heading on the service toolkit. https://www.gov.uk/service-toolkit

    The code for these pages can be found here: https://github.com/alphagov/product-page-example

    As part of this work we will make support easier to find and remove the beta phase banner

    The guidance link would go to: https://www.gov.uk/government/collections/digital-marketplace-buyers-and-suppliers-information

    The GOV.UK Digital Marketplace logo would go to the digital marketplace home page.

    Ticket ID: https://trello.com/c/mityCieG/106-update-header-to-make-it-easier-to-find-support-guidance-and-where-to-log-in
@DilwoarH DilwoarH force-pushed the dh-106-update-header-to-make-it-easier-to-find-support-guidance-and-where-to-log-in branch from 5d867c5 to 5524e15 Compare February 15, 2018 11:12
Copy link
Contributor

@idavidmcdonald idavidmcdonald left a comment

Choose a reason for hiding this comment

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

Woo great work Dilwoar!

@DilwoarH
Copy link
Contributor Author

🎉 FINALLY !

@DilwoarH DilwoarH merged commit 62e640e into master Feb 15, 2018
@DilwoarH DilwoarH deleted the dh-106-update-header-to-make-it-easier-to-find-support-guidance-and-where-to-log-in branch February 15, 2018 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants