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

Add parameter to localise mobile menu toggle button #2720

Merged
merged 5 commits into from
Aug 18, 2022

Conversation

domoscargin
Copy link
Contributor

@domoscargin domoscargin commented Jul 21, 2022

Adds the menuButtonText parameter to customise the text of the menu toggle button that appears on narrow screens.

Cherry picked this from @querkmachine's spike [#2614] as the minimum changes required.

Fixes #1904

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 July 21, 2022 11:44 Inactive
@domoscargin domoscargin added shared ownership design documentation User requests new documentation or improvements to existing documentation localisation labels Jul 21, 2022
@domoscargin domoscargin force-pushed the bk-header-menu-i18n branch from 7b1ab85 to ae748c7 Compare July 21, 2022 11:57
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 July 21, 2022 11:57 Inactive
@36degrees
Copy link
Contributor

HTML?

We've allowed for text replacement. I can't immediately think of any good reason to allow for HTML here - there may be an aspect of the i18n work I'm missing though?

I'd suggest using menuButtonText so that if we need to introduce the HTML option in the future we can do so whilst being consistent with the other options and components.

(Also works if we find we need to add e.g. menuButtonClasses or menuButtonAttributes)

@domoscargin domoscargin force-pushed the bk-header-menu-i18n branch from ae748c7 to fa18951 Compare July 21, 2022 14:14
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 July 21, 2022 14:15 Inactive
@querkmachine
Copy link
Member

In my redo of the footer parameters I did end up changing them from taking strings to following the whole html/text separation, as it let me merge some related parameters (like copyright link, image toggles) into the options.

I agree with @domoscargin that being able to set HTML here probably isn't needed. The main counterpoint I can think of is a situation where a language may have quite a long word(s) for navigation menu, so they opt to abbreviate it but then want a title/aria-label/visually-hidden text to expand upon it.

@36degrees
Copy link
Contributor

Just noticed we now have a navigationLabel and a menuButtonText that In most cases I suspect should be the same, and both default to 'Menu'. Suspect the navigationLabel might easily be missed when translating as it's not visible text.

Wondering if it might make sense to make navigationLabel default to whatever menuButtonText is, so that they stay in sync unless someone intentionally sets navigationLabel to something different? 🤔

@domoscargin domoscargin force-pushed the bk-header-menu-i18n branch from fa18951 to d5e5a1f Compare July 27, 2022 12:40
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 July 27, 2022 12:41 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 July 27, 2022 12:53 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 July 27, 2022 13:02 Inactive
@domoscargin
Copy link
Contributor Author

@36degrees, I've defaulted navigationLabel to menuButtonText.

I've also had a go at writing a changelog entry, but it feels a bit meh to just say "don't make it too long". Do we have any character length restrictions on other parameters?

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Changes all look sensible to me – my only suggestion is that it'd be good to have a test that the aria-label on the nav changes when menuButtonText is set.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 July 27, 2022 13:25 Inactive
@domoscargin
Copy link
Contributor Author

I'm thinking it'd be enough documentation-wise to edit the header.yaml menuButtonText description to something like:

Text for the button that toggles the navigation. This should be [the shortest useful text that describes the button's purpose][a maximum of 30 characters]. Defaults to 'Menu'.

@domoscargin domoscargin force-pushed the bk-header-menu-i18n branch from 614a6c1 to d2b2724 Compare July 27, 2022 20:05
@domoscargin domoscargin marked this pull request as ready for review July 27, 2022 20:05
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 July 27, 2022 20:05 Inactive
@querkmachine
Copy link
Member

To parrot what I've already said on Slack, for the permanent record:

I’m not sure of what guidance should go with this. There is a limit, but the limit is a visual one—you can’t really describe it as ‘max n characters’, especially in a localisation context

It’s heavily dependent on the width of the device too. If we use 320px as a baseline then you have a decent amount of space, at least.

@domoscargin
Copy link
Contributor Author

@christopherthomasdesign What do you think? Does this require more design work? Or will some kind of guidance do?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 August 10, 2022 08:36 Inactive
@christopherthomasdesign
Copy link
Member

@christopherthomasdesign What do you think? Does this require more design work? Or will some kind of guidance do?

I don't think so, long as we make it clear this is specifically to accommodate different languages. There seems to be enough space for a long-ish word to go alongside the logo. We just want to make sure users check the word they use still looks ok on smaller screens and at different zoom levels.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 August 15, 2022 13:11 Inactive
Copy link
Contributor

@vanitabarrett vanitabarrett left a comment

Choose a reason for hiding this comment

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

Looks good! Let's hold off merging until 4.3.1 is done

@domoscargin domoscargin added this to the [NEXT] milestone Aug 17, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2720 August 18, 2022 15:33 Inactive
@querkmachine
Copy link
Member

Moved the changelog entry as it was in the wrong location now that 4.3.1 has released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design documentation User requests new documentation or improvements to existing documentation localisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make 'menu' text in header customisable
6 participants