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

Change breadcrumb class name #435

Merged
merged 1 commit into from
Jul 20, 2018
Merged

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Jul 19, 2018

  • from 'gem-c-breadcrumbs--item' to 'gem-c-breadcrumbs__item'
  • was not following BEM convention, 'item' is an element, not a modifier

This should also fix the problem with this component in the 'highlight components' option of the GOV.UK browser extension, see below.

screen shot 2018-07-19 at 09 57 25


Component guide for this PR:
https://govuk-publishing-compon-pr-435.herokuapp.com/component-guide/

@benthorner benthorner temporarily deployed to govuk-publishing-compon-pr-435 July 19, 2018 08:58 Inactive
@andysellick andysellick force-pushed the breadcrumbs-rename-item-class branch from bb92b91 to cdcc184 Compare July 19, 2018 09:02
@benthorner benthorner temporarily deployed to govuk-publishing-compon-pr-435 July 19, 2018 09:14 Inactive
- from 'gem-c-breadcrumbs--item' to 'gem-c-breadcrumbs__item'
- was not following BEM convention, 'item' is an element, not a modifier
@@ -19,7 +19,7 @@
}

@include media-down(mobile) {
&.gem-c-breadcrumbs--collapse-on-mobile .gem-c-breadcrumbs--item {
&.gem-c-breadcrumbs--collapse-on-mobile .gem-c-breadcrumbs__item {
Copy link
Contributor

Choose a reason for hiding this comment

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

changing this class will mean all of these other repos will require a change aswell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eeek. Thanks, good spot.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a few apps - most of them look like prototypes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually looks like just collections. I'll raise a PR there to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DilwoarH have raised a PR in collections (alphagov/collections#796) - are you happy to approve this PR?

@andysellick andysellick merged commit 6f81e13 into master Jul 20, 2018
@andysellick andysellick deleted the breadcrumbs-rename-item-class branch July 20, 2018 09:59
This was referenced Jul 20, 2018
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