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

Topic list add branding #651

Merged
merged 4 commits into from
May 15, 2018
Merged

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented May 9, 2018

Add colour branding and a 'small' option to the topic list component.

Component guide: https://govuk-collections-pr-651.herokuapp.com/component-guide/topic-list

Trello card: https://trello.com/c/SvSNQlsa/50-modify-component-list-of-links-topic-list

@tijmenb tijmenb temporarily deployed to govuk-collections-pr-651 May 9, 2018 07:50 Inactive
@tijmenb tijmenb temporarily deployed to govuk-collections-pr-651 May 9, 2018 08:03 Inactive
@andysellick andysellick force-pushed the topic-list-add-branding-and-tracking branch from e1719f4 to fbcc041 Compare May 10, 2018 14:17
@tijmenb tijmenb temporarily deployed to govuk-collections-pr-651 May 10, 2018 14:17 Inactive
@andysellick andysellick force-pushed the topic-list-add-branding-and-tracking branch from fbcc041 to 927d66b Compare May 10, 2018 14:32
@tijmenb tijmenb temporarily deployed to govuk-collections-pr-651 May 10, 2018 14:32 Inactive
@andysellick andysellick requested a review from vanitabarrett May 10, 2018 14:57
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.

It probably makes sense to fixup the last commit (927d66b5d0f4b89811b233fd51b88d21d377bcdc) to the original commit that adds the branding. Other than that, looks good 👍

@@ -1,20 +1,22 @@
<%
items ||= []
see_more_link ||= false
size ||= false
component_size = "app-c-topic-list--small" if size == 'small'
Copy link
Contributor

Choose a reason for hiding this comment

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

Would passing in small: true make more sense here? Otherwise people might make typos and/or expect other sizes to be able to be passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha. I came up with both options but thought if I used small: true someone would say why not do a general one for size? But I guess we don't build things that we don't need yet, so I'll take a look.

On the fixup, I think I tried but it didn't work. I'll give it another attempt - I might be thinking of another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vanitabarrett I can't fixup that commit - I must have changed something that means the commits can't be reordered. Have addressed the other point though.

@andysellick andysellick merged commit bc4a031 into master May 15, 2018
@andysellick andysellick deleted the topic-list-add-branding-and-tracking branch May 15, 2018 10:26
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.

3 participants