search

adobe / helix-sidekick-extension

Browser extension for authors on AEM projects
Apache License 2.0
21 stars 39 forks source link

chore: add support for configurable badges #787

Closed mhaack closed 2 months ago

mhaack commented 3 months ago

Related Issues

fixes: #786

Config example for plugin list

 {
        "title": "PROD",
        "id": "env-sticker",
        "environments": ["edit", "dev", "preview", "live"],
        "isBadge": true,
        "badgeVariant": "green"
}
rofe commented 3 months ago

On second thought, maybe it's better (and less magic) to have explicit, optional properties:

rofe commented 3 months ago

Or a mix of both?

@dylandepass wdyt?

dylandepass commented 3 months ago

Or a mix of both?

  • badgeTextColor {string|string[]} The badge text color as either a hex string or an array (light and dark hex colors)
  • badgeBackgroundColor {string|string[]} The badge background color as either a hex string or an array (light and dark hex colors)

@dylandepass wdyt?

Fully explicit is better IMO. Easier to understand for developer and easier to support on our end.

mhaack commented 3 months ago

@dylandepass @rofe please have a look I updated it and we have now 4 dedicated color properties. All are optional. Not sure if there is a more elegant why to overwrite the color variables of the build in css, especially for dark mode.

mhaack commented 3 months ago

@rofe failing tests are not related to this PR and I see other PRs failing as well 🤷‍♂️

rofe commented 3 months ago

@rofe failing tests are not related to this PR and I see other PRs failing as well 🤷‍♂️

I know. Switch to cimg/node:20.4.0 as docker image in .circleci/config.yml. There seems to be an issue with node:current and our test setup.

rofe commented 3 months ago

@mhaack you can merge main into your branch to fix the tests.

rofe commented 3 months ago

Looking at the example config above, I'm wondering if we should simplify things for people who don't care about light or dark mode by adding 2 convenience properties:

The rules would be:

  1. If no colors are defined, the sidekick will decide which colors to use (as already implemented)
  2. If badge*Colors are defined, they override the sidekick default colors for both light and dark mode
  3. If badge*ColorLight or badge*ColorDark colors are defined, they override the badge*Color ones in the corresponding mode

@dylandepass @mhaack wdyt?

mhaack commented 3 months ago

@rofe We can simplify this a little and use badgeTextColor & badgeBackgroundColor for default aka. light mode as most people would consider light mode as the common case. So we would have

The rules would be:

dylandepass commented 3 months ago

@rofe @mhaack I'm wondering if we are complicating this all too much.. What about just using CSS variables and let the site change the variables as they seem fit.

--sidekick-badge-color: #fff;
--sidekick-badge-background-color: #f30;

Proposing --sidekick so we can keep the same naming for the new sidekick.

rofe commented 3 months ago

What about just using CSS variables and let the site change the variables as they seem fit.

How would you imagine the site doing that? Also, I think we should be careful not to override the badge colors globally, i.e. for all badges, but allow multiple badges with different colors.

dylandepass commented 3 months ago

How would you imagine the site doing that?

Sites can overrride in their styles.css

helix-sidekick {
  --sidekick-badge-color: #f30;
}

Also, I think we should be careful not to override the badge colors globally, i.e. for all badges, but allow multiple badges with different colors.

Yeah if multiple badges with different colors is a thing (is it?) then this idea won't work.

rofe commented 3 months ago

This approach also wouldn't work in SharePoint or on any non-HTML resources. I'd stick with a plugin config based solution.

mhaack commented 3 months ago

@dylandepass

What about just using CSS variables and let the site change the variables as they seem fit.

This will not work while in word. Very initially I added a sticker only via scripts.js but ended with the same chellange, word.

Will update the PR and use the simplified config names https://github.com/adobe/helix-sidekick-extension/pull/787#issuecomment-2326494824

mhaack commented 3 months ago

@rofe @dylandepass we have now only

Test got updated with 3 badges: default colors, one set of colors configured and two set of colors configured to test all cases. As discussed with @rofe on Slack, for badge foo3 I currently test only the light colors as getComputedStyle(badge) always return the active colors.

mhaack commented 2 months ago

I triede to force the test chrome into dark mode with --enable-features=WebContentsForceDark option. However this does not work fully and window.matchMedia('(prefers-color-scheme: dark)').matches; still returns false also other CSS for dark mode is not applied with that correctly. See screenshot:

Screenshot 2024-09-05 at 10 35 57

If I put my mac into dark mode it works without any issues:

Screenshot 2024-09-05 at 10 39 18
rofe commented 2 months ago

@mhaack change of plans after a review with Kilian: he would like us to be able to control the badge colors rather than letting users pick whatever they want (potentially resulting in a bad user experience). He is proposing to use a fixed set of spectrum colors.

In order to keep the sidekick plugin API forward compatible, this means we'll need to switch to predefined colors and a single badgeColor property in the API (gray being the default if none is selected). Kilian will provide the necessary color values for both dark and light mode.

cc @dylandepass

mhaack commented 2 months ago

@rofe PR got update and reworked to use the Spectrum Badge variants Standard & Non-Semantic as discussed with Killian.

Config now accepts a badgeVariant only. With all the variant options, using an invalid variant name it will fall back to default (neutral)

trieloff commented 2 months ago

:tada: This PR is included in version 6.49.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: