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 ability to brand top left header #281

Merged
merged 17 commits into from
Oct 23, 2019

Conversation

jaredami
Copy link
Contributor

@jaredami jaredami commented Oct 18, 2019

Closes #245

This PR adds the ability to apply a branded background to the top left section of the header.

Our GoConfigService already provides the ability to set a branding color. This PR adds the ability to toggle on and off whether this branding is applied to the header via the service's toggleHeaderBrandingEnabled method.

The header will use the service to ensure that the contrast ratio between the branded background and the color of the menu icon remain accessible by utilizing its contrastIsAccessible method.

@jaredami jaredami changed the title Feature ability to brand top left header Add ability to brand top left header Oct 18, 2019
@grahamhency grahamhency added this to the v1.3.0 milestone Oct 21, 2019
@jaredami jaredami added the feature New feature request label Oct 21, 2019
@jaredami jaredami requested a review from grahamhency October 21, 2019 14:26
Copy link
Contributor

@grahamhency grahamhency left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good.

Should we get some tests around this stuff?

Got an issue with the icons in the buttons, https://deploy-preview-281--romantic-khorana-d856b1.netlify.com/ui-kit/button.

projects/go-lib/src/lib/go-config.service.ts Show resolved Hide resolved
@jaredami jaredami requested a review from grahamhency October 21, 2019 19:03
@jaredami
Copy link
Contributor Author

@grahamhency I added tests for the GoConfigService and fixed the issue with the buttons you mentioned.

Copy link
Contributor

@grahamhency grahamhency left a comment

Choose a reason for hiding this comment

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

Hey @jaredami, I think we can simplify this icon issue further. Check my other comment.

@@ -32,21 +32,21 @@ $icon-sizes: (small: 1rem, medium: 1.5rem, large: 2rem);
}

.go-icon--dark {
color: $theme-dark-color;
color: $theme-light-color;
Copy link
Contributor

Choose a reason for hiding this comment

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

@jaredami I think instead of changing this. We can just remove the [iconModifier] being passed to the go-icon by the go-button. See line 18 in go-button.component.html. The inheritance should take over here.

@grahamhency grahamhency mentioned this pull request Oct 22, 2019
@jaredami jaredami requested a review from grahamhency October 22, 2019 19:10
Copy link
Contributor

@grahamhency grahamhency left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@grahamhency grahamhency mentioned this pull request Oct 23, 2019
@grahamhency grahamhency merged commit 5e68ded into v1.3.0 Oct 23, 2019
@grahamhency grahamhency deleted the feature_ability_to_brand_top_left_header branch October 23, 2019 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants