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

fix(widgets): widget style prop keys should accept camelCase css properties and dashed css variables #8991

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

TylerMatteo
Copy link
Contributor

@TylerMatteo TylerMatteo commented Jun 29, 2024

Closes #8990

Change List

  • Updated the style prop on CompassWidget, ZoomWidget, and FullscreenWidget to support camelCase CSS properties (e.g. backgroundColor) and kabab-case CSS variables (e.g. --button-size).

@TylerMatteo TylerMatteo force-pushed the tm/dashcase-widget-style branch 2 times, most recently from b9bd9a3 to d505e3f Compare July 3, 2024 15:18
@felixpalmer felixpalmer requested a review from chrisgervang July 4, 2024 07:36
@coveralls
Copy link

Coverage Status

coverage: 89.216%. remained the same
when pulling d505e3f on TylerMatteo:tm/dashcase-widget-style
into 31ff4bd on visgl:master.

@Pessimistress
Copy link
Collaborator

@chrisgervang if I remember right, we used to use Object.assign(element.style, style) which only worked for camel case property names. It was switched to element.style.setProperty in order to support CSS variables.

Alternatively we could just do this the dirty way by calling both of the above...

@chrisgervang
Copy link
Collaborator

@Pessimistress, that's correct, but I didn't account for camelCase styles. It's probably best to have a solution that supports at least variables and camelCase, or all three (even if kebab-case isn't popular).

I think there are three solutions to consider.

  1. Simple to understand and supports variables and camelCase:
if (property.startsWith('--')) {
  // Assume CSS variable
  element.style.setProperty(property, value);
} else {
  // Assume camelCase
  element.style[property] = value;
}
  1. The next being suggested by this PR. @TylerMatteo's solution works with all three input types. It's pretty good, but asking every widget author to understand regex isn't my preference.

  2. Being explicit with handling each input.

if (property.startsWith('--')) {
  // Assume CSS variable
  element.style.setProperty(property, value);
} else if (property.includes('-')) {
  // Convert kebab-case to camelCase
  const camelCaseProperty = property.replace(/-([a-z])/g, (match, letter) => letter.toUpperCase());
  element.style[camelCaseProperty] = value;
} else {
  // Assume camelCase
  element.style[property] = value;
}

I've uploaded them into a gist for easy testing.


While element.style[property] = value; sometimes works with kebab-case in Chrome, it's not a standard behavior we should rely on. I think I'd prefer to go with option 1 unless there's a strong argument to support camelCase and kebab-case in JS. Core widgets are meant to be examples in themselves, so I'd like to keep them simple to read and understand.

@TylerMatteo TylerMatteo force-pushed the tm/dashcase-widget-style branch from d505e3f to 70b961f Compare July 7, 2024 17:49
@TylerMatteo
Copy link
Contributor Author

@chrisgervang sounds reasonable to me - I hadn't considered css variables. Shall I go ahead and update my branch to implement solution 1?

@chrisgervang
Copy link
Collaborator

Yes, thank you! Could you also please take a look at the style prop docs to see if they need clarification?

@Pessimistress
Copy link
Collaborator

It might be easier to create a shared function applyStyles(element, styles).

@TylerMatteo TylerMatteo force-pushed the tm/dashcase-widget-style branch from 9e75fc8 to 456d1eb Compare July 28, 2024 16:44
@TylerMatteo
Copy link
Contributor Author

@chrisgervang I made that change and created a shared applyStyles util added to core. Let me know if you'd prefer this change stay scoped to the widgets module.

Copy link
Collaborator

@chrisgervang chrisgervang left a comment

Choose a reason for hiding this comment

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

Great progress! Left some comments. @Pessimistress would be the one to make a call on whether she wants these utility functions in core or not. I think we could consider whether or not it'd be useful to use the Deck class too.

test/modules/core/utils/apply-styles.spec.ts Show resolved Hide resolved
Comment on lines 100 to 108
Object.entries(oldProps.style).map(([key]) => {
if (key.startsWith('--')) {
// Assume CSS variable
el.style.removeProperty(key);
} else {
// Assume camelCase
el.style[key] = '';
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's create a removeStyles as well. This seems useful to apply in all widgets when style has changed

@coveralls
Copy link

coveralls commented Aug 13, 2024

Coverage Status

coverage: 91.707% (+0.005%) from 91.702%
when pulling 62a3541 on TylerMatteo:tm/dashcase-widget-style
into e9eada6 on visgl:master.

@chrisgervang chrisgervang mentioned this pull request Dec 7, 2024
45 tasks
@chrisgervang chrisgervang changed the title fix(widgets): convert widget style prop keys from camel to dash case fix(widgets): widget style prop keys should accept camelCase css properties and dashed css variables Dec 12, 2024
@chrisgervang chrisgervang merged commit 691baea into visgl:master Dec 17, 2024
4 checks passed
felixpalmer pushed a commit that referenced this pull request Jan 15, 2025
…erties and dashed css variables (#8991)

* fix(widgets): handle css variable and camel cased keys passed to widget style prop

---------

Co-authored-by: Chris Gervang <[email protected]>
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.

[Bug] Widget style prop does not apply styles for camel cased style keys
4 participants