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

ui: Fix brand coloring for inline-code plus docs #11578

Merged
merged 6 commits into from
Nov 23, 2021
Merged

Conversation

johncowen
Copy link
Contributor

I noticed the coloring of our %inline-code style wasn't picking up the Consul brand color. So this PR fixes that up by also moving --brand to use a --tone- prefix and require rgb like all our other colors, which fixes that up.

Before:

Screenshot 2021-11-16 at 15 29 52

After:

Screenshot 2021-11-16 at 15 29 25

I also took the opportunity to add some documentation explaining all of the tone/rgb thing plus all of our colors swatches. I made these easier to copypasta using our with-copyable modifier. So you can click on these swatches to get the right thing to paste.

While I was there I did the same for all our icons and typography, which makes a good reference/document for when we need to tweak all this.

@johncowen johncowen added the theme/ui Anything related to the UI label Nov 16, 2021
@johncowen johncowen added this to the 1.11.0 milestone Nov 16, 2021
@johncowen johncowen requested a review from kaxcode November 16, 2021 15:32
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging November 16, 2021 15:34 Inactive
@vercel vercel bot temporarily deployed to Preview – consul November 16, 2021 15:35 Inactive
@johncowen johncowen requested a review from evrowe November 22, 2021 17:45

```hbs preview-template
<ul
{{css-props (set this 'tones')
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ha, so this is what that modifier does. Cool 😎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😸 well kinda, tbh this one started out as a "only give me the variables for this element" - the important bit being "for this element". This one turned into getting things from the root element (not the modified element) so 'theoretically' shouldn't be a modifier - in the scheme of things (and as its only for documentation purposes) I just left it as is. I may move it to be a helper at some point but no biggie really (unless we use it in app code)

I always decide on a modifier if ever the "I just need a reference to the DOM element" slips into my head. The below with-copyable is probably a better example.

Copy link
Contributor

@evrowe evrowe left a comment

Choose a reason for hiding this comment

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

Just need to address the minor typos in docs and should be good to ship 👍🏻

* Moves almost all instances of --black/--white to use --tone

* Spotted that we are doubling up on yellows
@hashicorp-cla
Copy link

hashicorp-cla commented Nov 22, 2021

CLA assistant check
All committers have signed the CLA.

@johncowen
Copy link
Contributor Author

K those are in @evrowe gimme a yell/ 👍 when you are around and we can get this one in!

Copy link
Contributor

@evrowe evrowe left a comment

Choose a reason for hiding this comment

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

🚀

@johncowen johncowen merged commit b84ee47 into main Nov 23, 2021
@johncowen johncowen deleted the ui/chore/color-docs branch November 23, 2021 18:32
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/507889.

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.

4 participants