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

Fix/remove current page breadcrumb #872

Merged
merged 7 commits into from
Jul 4, 2023

Conversation

the-nathan-smith
Copy link
Contributor

Description

Removed the need to add the last breadcrumb outside of the 'Items' list, now simply include it in the list of items. This also fixes the issue (Issue 471 in the nhsuk Service Manual) with not being able to add attributes to the last breadcrumb.

Checklist

Copy link
Contributor

@andymantell andymantell left a comment

Choose a reason for hiding this comment

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

Approved - seems like a nice cleanup.

@andymantell
Copy link
Contributor

Could we maybe do it in a backward compatible way? Leave the old params in and use them if they're present. Otherwise use the new way.

Change the docs to encourage the new way, but allow the old way for a while, until we actually do another major release. It just feels like such a small change to push out a whole major version.

Copy link
Contributor

@mayank1211 mayank1211 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 to me. Probably worth looking into what Andy said about backward compatibilities.

@pflynny
Copy link
Contributor

pflynny commented Jul 4, 2023

LGTM. Nice work @the-nathan-smith

@the-nathan-smith the-nathan-smith merged commit 9b851f8 into main Jul 4, 2023
@the-nathan-smith the-nathan-smith deleted the fix/remove-current-page-breadcrumb branch July 4, 2023 15:47
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.

4 participants