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

Mobile breadcrumb update guidance #1298

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

RobBond-GDS
Copy link
Contributor

On mobile devices update breadcrumb behaviour as per :

https://trello.com/c/YuObLgSq/10-fix-breadcrumbs-on-mobile

What

Update guidance documentation to remove patterns with "is_current_page"

Why

When a breadcrumb on a mobile device is collapsed, if last item is set as "is_current_page" then it is not possible to navigate to parent items. To eliminate the inconsistency, this option has been removed

@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 17, 2020 10:54 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 17, 2020 11:05 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 17, 2020 12:22 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 17, 2020 12:40 Inactive
@RobBond-GDS RobBond-GDS force-pushed the mobile-breadcrumb-update-guidance branch from 1dfc217 to 22c0408 Compare February 17, 2020 12:42
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 17, 2020 12:42 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.

The logic which handles this flag is still present and needs to be removed, e.g: https://github.com/alphagov/govuk_publishing_components/blob/master/lib/govuk_publishing_components/presenters/breadcrumbs.rb#L46

We should double check we're not using this flag anywhere before removing it.

@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 09:48 Inactive
@RobBond-GDS RobBond-GDS force-pushed the mobile-breadcrumb-update-guidance branch from 5be30a3 to 341f002 Compare February 18, 2020 09:56
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 09:56 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.

@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 12:38 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 13:16 Inactive
@RobBond-GDS RobBond-GDS force-pushed the mobile-breadcrumb-update-guidance branch from af6e283 to c35bc61 Compare February 18, 2020 13:18
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 13:18 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 14:39 Inactive
@RobBond-GDS RobBond-GDS force-pushed the mobile-breadcrumb-update-guidance branch from d565589 to 0b009ce Compare February 18, 2020 14:45
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 14:45 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 15:51 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 15:53 Inactive
@RobBond-GDS RobBond-GDS force-pushed the mobile-breadcrumb-update-guidance branch from c9029ce to 167e8ce Compare February 18, 2020 16:38
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 18, 2020 16:39 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 19, 2020 08:50 Inactive
@vanitabarrett
Copy link
Contributor

@RobBond-GDS Looks good, but not sure why there's a merge commit here - can we rebase to remove it?

- Added instructions to specify behaviour on devices less than "tablet" width
- Removed Collapse on Mobile, Highlight current page and last breadcrumb
- Removed is_current_page flag
- Removed all reference to request_path
- Update change log
@RobBond-GDS RobBond-GDS force-pushed the mobile-breadcrumb-update-guidance branch from b3bcc97 to acd01cc Compare February 19, 2020 09:58
@bevanloon bevanloon temporarily deployed to govuk-publis-mobile-bre-rrojd1 February 19, 2020 09:58 Inactive
@RobBond-GDS
Copy link
Contributor Author

RobBond-GDS commented Feb 19, 2020 via email

@RobBond-GDS RobBond-GDS merged commit 6ffd64b into master Feb 19, 2020
@RobBond-GDS RobBond-GDS deleted the mobile-breadcrumb-update-guidance branch February 19, 2020 10:09
@huwd huwd mentioned this pull request Feb 19, 2020
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.

5 participants