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

Remove hard-coded text-color from recent posts entries. #813

Closed
wants to merge 2 commits into from

Conversation

nulvox
Copy link

@nulvox nulvox commented Jan 12, 2025

The old implementation directly set the text color dark-gray which conflicts with many of the background colors the user can set with the background_color_class param. Since the similar param background_clases is honored in this object, so should the similar params post_content_closesses and/or text_color as would otherwise apply.

Copy link

netlify bot commented Jan 12, 2025

Deploy Preview for ananke-theme failed.

Name Link
🔨 Latest commit 7570c37
🔍 Latest deploy log https://app.netlify.com/sites/ananke-theme/deploys/6784306010b5df00082f22fa

@nulvox
Copy link
Author

nulvox commented Jan 12, 2025

This error is unclear to me. Though it reports that it is user error, I do no explicitly pass "upgrade" anywhere in this PR. Could I have some help resolving this?

4:13:50 PM: Error: failed to load modules: failed to get ["-d" "github.com/theNewDynamic/gohugo-theme-ananke@upgrade"]: failed to execute 'go [get -d github.com/theNewDynamic/gohugo-theme-ananke@upgrade]': failed to execute binary "go" with args [get -d github.com/theNewDynamic/gohugo-theme-ananke@upgrade]: go: github.com/theNewDynamic/gohugo-theme-ananke@upgrade: no matching versions for query "upgrade"

@davidsneighbour
Copy link
Collaborator

Ignore the error. Subsequent deploys should work again.

I can't/won't merge this. Close this PR, open an issue, and I'll rework the way this is done. Or: add the FULL changes to this PR and I'll merge.

This PR should contain:

  • adding a new config setting for text color with a default of the current version
  • documenting the config setting
  • removing the old way

@nulvox
Copy link
Author

nulvox commented Feb 9, 2025

Sounds good, I'll work on bringing this into spec next time I'm making an update to the site that's using it.

@nulvox nulvox closed this Feb 9, 2025
@davidsneighbour
Copy link
Collaborator

I was thinking it makes most sense to add one single class config to add background and text classes and then give that one a default for both. They will be in the same class="" attribute in any case, so adding two config options seems a bit cluttered. Better rename the existing one from a background class option to a class option.

@nulvox
Copy link
Author

nulvox commented Feb 10, 2025

Im not completely sure I understand what you mean, but I'd love to make the value adjustable without conflicting with the current coding style.

Do you mean any of these or something else?

  1. Text and background color should be controlled with 1 option in the recent post widget (removes some expression, but avoids complexity)

  2. The settings in this widget are somehow adjusted by the top level page configurations .

Im ready to work out either of those or if those were both outside what you meant Id love to talk that out more specifically.

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.

2 participants