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

Contrast colors added to theme-colors #30044

Merged
merged 5 commits into from
Oct 31, 2020

Conversation

memic84
Copy link
Contributor

@memic84 memic84 commented Jan 17, 2020

Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

Only change is the background of the yellow text: https://deploy-preview-30044--twbs-bootstrap.netlify.com/docs/4.3/utilities/colors/#color (but this is an improvement)

Copy link
Member

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

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

  1. I'd change contrast_color to contrast-color
  2. I wouldn't add redundant classes; we only need this in 2 cases otherwise it doesn't have any effect

@XhmikosR XhmikosR added the docs label Jan 17, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Jan 17, 2020

@memic84 you can use something like (untested) {{ with .constrast-ratio }} text-{{ . }}{{ end }}; this will only add the extra classes if they are set. Then you can remove the unneeded ones from the yml file.

@memic84
Copy link
Contributor Author

memic84 commented Jan 17, 2020

I renamed the class to contrast-color, but there are problems with the proposed change for redundant classes.

The cards also require the white color to be set, which means that we have to set all contrast-colors. Also this will add more logic, and the logic will be in two places templates and theme-colors, where with the redundant classes it's only in theme-colors.yml.

@XhmikosR
Copy link
Member

Yeah, but the end users don't need the extra classes.

Anyway, I'll have a look later myself and see how this can be simplified.

@memic84
Copy link
Contributor Author

memic84 commented Jan 17, 2020

Yeah, but the end users don't need the extra classes.

Anyway, I'll have a look later myself and see how this can be simplified.

I agree, that the end users don't need the extra classes.

My only problem is that, for example the contrast-color of primary, is sometimes needed and sometimes not. I will check the Hugo docs, for a solution...

@XhmikosR
Copy link
Member

Maybe we should go with your original patch and just mention that the extra classes are not needed?

But then if we are going to do that, we might as well just go the with solution since we'll edit the files.

BTW contrast-color doesn't work with Hugo, I will revert it to the underscore one.

@XhmikosR XhmikosR force-pushed the master-ms-contrast-colors-theme-colors branch from d5aa7ac to 5a1f0ff Compare January 17, 2020 14:10
@XhmikosR
Copy link
Member

This is what I suggested in case you want to play with it {{ with .contrast_color }} text-{{ . }}{{ end }} (adapt it for the bg case as needed).

But we need to think how to proceed before any further changes.

@XhmikosR
Copy link
Member

Also, found a bug with Hugo; when I change the data file, the changes are not reflected unless I restart Hugo. I will try to report it.

@memic84
Copy link
Contributor Author

memic84 commented Jan 17, 2020

This is what I suggested in case you want to play with it {{ with .contrast_color }} text-{{ . }}{{ end }} (adapt it for the bg case as needed).

But we need to think how to proceed before any further changes.

With this it means, that the contrast color will be used if there's one defined in the theme-colors.yml?

It seems like a solid solution, and that we create the exception in templates where needed, ex. cards with text-white

@XhmikosR
Copy link
Member

XhmikosR commented Jan 17, 2020

With this it means, that the contrast color will be used if there's one defined in the theme-colors.yml?

Yeah, with is just for that. See https://gohugo.io/functions/with/

I mean, if we are going to edit the content files to say that the extra classes are only needed for our examples, we might as well go with this solution since we will need to edit the files anyway.

Another solution would be to try to abstract the inline shortcode's logic into a common shortcode where we will pass HTML etc. I haven't tried this so I don't know if it's worth it, but it might be simpler to have the logic in one place, assuming it works.

@memic84
Copy link
Contributor Author

memic84 commented Jan 17, 2020

This works: <span class="badge bg-{{ .name }}{{with .contrast_color}} text-{{.}}{{end}}">{{ .name | title }}</span>{{- end -}} for example in the badge page.

I'll push soon...

@memic84
Copy link
Contributor Author

memic84 commented Jan 17, 2020

There might be some inconsistencies in hugo templates with coding style. I'll fix this soon, when the proposed fix is ok.

@memic84
Copy link
Contributor Author

memic84 commented Jan 29, 2020

@XhmikosR Have you checked the changes? Let me know is this is OK now?

@ffoodd
Copy link
Member

ffoodd commented Apr 10, 2020

FYI this'll have conflicts with #30468 : if we ever merge this one, simply drop my own changes in .md files since they basically add a utility to use sufficiently contrasted colors.

@ffoodd
Copy link
Member

ffoodd commented Apr 17, 2020

LGTM. @XhmikosR, anything else to update?

@XhmikosR
Copy link
Member

Don't merge this yet, I haven't cleaned it up

@mdo mdo changed the base branch from master to main June 16, 2020 20:12
@mdo
Copy link
Member

mdo commented Oct 29, 2020

What's the cleanup left here @XhmikosR?

@XhmikosR XhmikosR force-pushed the master-ms-contrast-colors-theme-colors branch from d1ffa50 to 6e96f0a Compare October 30, 2020 13:11
@XhmikosR XhmikosR force-pushed the master-ms-contrast-colors-theme-colors branch from a2ac5da to cfc457b Compare October 30, 2020 13:19
@XhmikosR
Copy link
Member

@mdo @ffoodd please have another look. I rebased the branch and adapted it to the recent changes.

One way would be to have light/dark in all colors, or the other would be what we currently have by specifying only the exceptions.

Regardless, needs another pair of eyes since I didn't test it thoroughly and it's been a long time since the PR was made :)

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

LGTM, very elegant (and efficient!)

And specifying the exceptions seems better to me, since this is what we need this for. MAkes it much easier to spot colours with specific requirements.

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add contrast colors to theme-colors.yml
5 participants