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

Update brand assets #10081

Merged
merged 8 commits into from
May 3, 2021
Merged

Update brand assets #10081

merged 8 commits into from
May 3, 2021

Conversation

dizzyup
Copy link
Contributor

@dizzyup dizzyup commented Apr 21, 2021

This PR introduces updated brand assets for Consul (against v1.10).

There's also a version of this same PR against v1.9.3 - #10090

Note
This isn't intended to go live until the wider rollout happens which is being handled by marketing - I'd like to get this work reviewed though so we can merge once marketing ship the other Consul properties.

Changelog

  • Updated header logo
  • Updated attribution icon
  • Updated favicon and meta assets
  • Updated logos for loading states

As part of the favicon/meta work I took the opportunity to update the strategy there so we serve a favicon.svg alongside a favicon.ico file, together with an Apple Touch Icon asset.

CleanShot 2021-04-21 at 12 04 43@2x

CleanShot 2021-04-21 at 12 14 14@2x

CleanShot 2021-04-21 at 12 21 33@2x

CleanShot 2021-04-21 at 16 17 13@2x

@dizzyup dizzyup requested review from johncowen and kaxcode April 21, 2021 15:38
@dizzyup dizzyup self-assigned this Apr 21, 2021
@github-actions github-actions bot added the theme/ui Anything related to the UI label Apr 21, 2021
@dizzyup dizzyup requested a review from mterich April 21, 2021 15:47
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 21, 2021 16:02 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 21, 2021 16:02 Inactive
Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

Hey @dizzyup

I know you are still working on this, but I noticed a couple of things I thought I'd mention.

Firstly, oh my wordy 😍 !

Also, thanks for cleaning up all those extraneous files!

Couple of things I wanted to ask were:

  • Is the SVG you are using 'minizable' at all, can we use svgo on it?
  • I noticed the logo uses the newer color scheme also, I can think of one place off the top of my head where we use a shade of the older magenta scheme, is it worthwhile tweaking that in this PR also? (I just merged a PR that would affect where that would be done, I'm just gonna look now so I can point you to it if you wanted to do that here - gah I just thought, it would be slightly different for the 2 branches also) Anyway let me know if you want to do that here at all, or me or you do it separately.

@dizzyup
Copy link
Contributor Author

dizzyup commented Apr 22, 2021

@johncowen No problem!

  • Is the SVG you are using 'minizable' at all, can we use svgo on it?

So the asset has actually already gone through SVGO before I embed the markup - the larger size is because the new SVG has more points along the paths. I'm not sure there's any further optimisation that can happen there.

  • I noticed the logo uses the newer color scheme also ... is it worthwhile tweaking that in this PR also?

It might be, though I was steering clear of wider design changes to make sure just the brandmark was the thing being landed. What would the change affect specifically? The marketing guidelines for the brand give us #E03875 for the logo, and #DC477D which is intended for buttons and UI elements, but there are additional shades that are context specific - we'd likely need to make some judgement calls so I figured I'd save those for a later effort.

@johncowen
Copy link
Contributor

So the asset has actually already gone through SVGO before I embed the markup

Oh cool, would it be ok to make a app/components/consul/logo/index.hbs template-only component for it so we can tuck it out the way? Then it'd be just that little bit less 'wow numbers!' when you open the hashicorp-consul component hbs file. If not no worries.

What would the change affect specifically?

We use it for the underline highlights for our tabs, and then a tiny detail of code syntax highlighting for our inline code style. We can change that up at a later date tho if thats better.

@dizzyup
Copy link
Contributor Author

dizzyup commented Apr 22, 2021

Oh cool, would it be ok to make a app/components/consul/logo/index.hbs template-only component for it..

Great idea, will do that now!

We use it for the underline highlights for our tabs, and then a tiny detail of code syntax highlighting for our inline code style. We can change that up at a later date tho if thats better.

I'm not against that at all! Do you want to try applying #DC477D in a commit?

@johncowen
Copy link
Contributor

Yeah sure, could you rebase this one for me and I'll make a little branch off here

Comment on lines +7 to +9
.is-debug .brand-loader {
display: none !important;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johncowen Given that the eng docs only show locally, I think this solution is ok because this style is only loaded when we're a) local, b) looking at the docs and c) the docs css has been loaded (so we can hide the loading state).

Have I missed anything with this approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think so, thanks for extra explanation!

Ping me when this is out of draft and I can give it a 👍

Comment on lines +12 to +14
[role='banner'] nav:first-of-type {
height: calc(100vh - var(--chrome-height, 47px));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johncowen Noticed in the eng docs that the left-hand navigation had a gap at the bottom. This rule is a version of the standard one but removes the - 35px portion of that calc.

Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

Super good! Thanks for the tweaks there too!

👍

@dizzyup dizzyup merged commit 211bc82 into master May 3, 2021
@dizzyup dizzyup deleted the logo-asset-update branch May 3, 2021 15:19
dizzyup added a commit that referenced this pull request May 5, 2021
* Update header logo and inline icon

* Update full logos + layout on loading screen

* Update favicon assets and strategy
- Switches to serve an ico file alongside an SVG file
- Introduces an apple-touch-icon

* Removes unused favicon/meta assets

* Changelog item for ui

* Create component for logo

* Simplify logo component, set brand color

* Fix docs loading state CSS issue
mikemorris pushed a commit that referenced this pull request May 5, 2021
* Update header logo and inline icon

* Update full logos + layout on loading screen

* Update favicon assets and strategy
- Switches to serve an ico file alongside an SVG file
- Introduces an apple-touch-icon

* Removes unused favicon/meta assets

* Changelog item for ui

* Create component for logo

* Simplify logo component, set brand color

* Fix docs loading state CSS issue
mikemorris pushed a commit that referenced this pull request May 5, 2021
* Update header logo and inline icon

* Update full logos + layout on loading screen

* Update favicon assets and strategy
- Switches to serve an ico file alongside an SVG file
- Introduces an apple-touch-icon

* Removes unused favicon/meta assets

* Changelog item for ui

* Create component for logo

* Simplify logo component, set brand color

* Fix docs loading state CSS issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants