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 the ability to change the primary color of the comments widget and other styles #1633

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

husky-dev
Copy link

First of all, thank you for this comments engine. I have added it to my blog, and it looks great. However, I have not found a way to change the primary color. The default green color is not the best choice for my website, and it would be better to use blue as the primary color for https://radio-t.com/. Therefore, I have decided to add this functionality.

2023-05-02 14 15 04

This PR adds the ability to change the primary color by using remark_config:

var remark_config = {
  // ...other configs
  styling: {
    colors: {
      primary: {
        light: '#0aa',
        dark: '#099',
      },
    },
  },
};

Or change it dynamically:

window.REMARK42.changeStyling({
  colors: {
    primary: '#099',
  },
})

Initially, I planned to use the theme config key for this feature, but it is already used for selecting a theme (light/dark). Although it is possible to use this key with backward compatibility, such as light, dark, or {colors: {}}. But it might overcomplicate the source code. Additionally, the styles key is used for iframe's custom styles. Therefore, I decided to use the styling key instead.

I saw some commented code which indicated that the initial plan was to add something like a __colors__ key and allow the user to provide colors by name, such as --color0, --color9, and so on. However, it might be difficult for the user to change each color in this way. It would be much easier to provide one primary color and generate the required tones.

The config object data structure is designed to be easily extensible in the future, allowing for the addition of properties such as text color, background color, font configurations, and more. However, I have not implemented all of this functionality because I would like to hear your feedback first.

The code applies styles by changing appropriate CSS variables.

The code allows for the application of a single color for both themes (like: primary: '#099') or the use of separate colors for each (primary: {light: '...', dark: '...'}). This may not be as useful for the primary color, but it can be very useful for the background color, which might be completely different in those cases.

I've added some utilities to make type checking easier (app/utils/types.ts), which I'm using in the type guards, as well as a utility for parsing and working with colors (app/utils/colors.ts). This is not difficult to implement, so I decided not to add external dependencies.

Please let me know your thoughts, as I am open to any constructive criticisms or suggestions you may have.

@husky-dev husky-dev requested a review from umputun as a code owner May 2, 2023 12:20
@paskal paskal requested a review from akellbl4 June 12, 2023 06:30
@paskal
Copy link
Collaborator

paskal commented Jun 12, 2023

I can't check the technical implementation, but the idea looks great and simple enough to be used by many users potentially.

@akellbl4 can you please take a look?

@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Patch coverage: 67.96% and project coverage change: +0.77 🎉

Comparison is base (07667c8) 58.63% compared to head (0ca7875) 59.41%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1633      +/-   ##
==========================================
+ Coverage   58.63%   59.41%   +0.77%     
==========================================
  Files         128      134       +6     
  Lines        2877     3075     +198     
  Branches      730      789      +59     
==========================================
+ Hits         1687     1827     +140     
- Misses       1065     1114      +49     
- Partials      125      134       +9     
Impacted Files Coverage Δ
frontend/apps/remark42/app/embed.ts 0.00% <0.00%> (ø)
frontend/apps/remark42/app/remark.tsx 0.00% <0.00%> (ø)
...rontend/apps/remark42/app/store/styling/actions.ts 0.00% <0.00%> (ø)
frontend/apps/remark42/app/utils/create-iframe.ts 0.00% <0.00%> (ø)
frontend/apps/remark42/app/utils/post-message.ts 66.66% <ø> (ø)
frontend/apps/remark42/app/common/theme.ts 48.00% <48.00%> (ø)
...ontend/apps/remark42/app/store/styling/reducers.ts 75.00% <75.00%> (ø)
frontend/apps/remark42/app/common/settings.ts 91.30% <100.00%> (+0.82%) ⬆️
frontend/apps/remark42/app/common/types.ts 100.00% <100.00%> (ø)
frontend/apps/remark42/app/store/reducers.ts 100.00% <100.00%> (ø)
... and 3 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@akellbl4
Copy link
Collaborator

akellbl4 commented Oct 28, 2023

@husky-dev hey, sorry I'm coming to late here but we have ability to pass css variables of colors in JS config. Have you seen that? https://github.com/umputun/remark42/blob/master/frontend/apps/remark42/templates/demo.ejs#L129 this is the thing, it's not documented but we are not gonna chage it if we don't go by any other styling route.

I like the idea with color picker, I'll check it out this weekend

@husky-dev
Copy link
Author

@akellbl4 hey, yes, I saw an example with __colors__. My idea was to implement theme configuration similar to popular UI libraries, with primary and secondary colors, the ability to change fonts, and so on. That's why my proposed config looks like this:

var remark_config = {
  // ...other configs
  styling: {
    colors: {
      primary: {
        light: '#0aa',
        dark: '#099',
      },
    },
    // ...fonts,
    // ...icons,
    // ...etc
  },
};

Here is the current implementation:

var remark_config = {
  site_id: 'remark',
  host: '<%= htmlWebpackPlugin.options.REMARK_URL %>',
  url: window.location.href,
  components: ['embed', 'counter'],
  // __colors__: {
  //   "--color0": "red",
  // },
}

The idea was to set one base color, and other colors would be calculated from it. Right now, there's a bunch of different colors and their hues, and it's unclear which is responsible for what. Names like color0, color1, and color2 don't help with that. I think users of the widget won't understand why the property is named __colors__ with underscores, while other props aren't. It looks like a hack. What if you want to add font configuration because your website uses custom fonts? With the current code style, you'll probably have to add a __fonts__ prop. I don't think it's a good idea to use separate config props when most libraries simply use a theme prop. Since theme is already taken, the next logical choice was styling. This way, you could consolidate all styling configurations under a single prop, like colors, fonts, icons, and so on.

But this PR has been open for almost 6 months, and I'm not sure that I want to keep working on it anymore. So feel free to close it.

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.

4 participants