deck.gl icon indicating copy to clipboard operation
deck.gl copied to clipboard

fix(widgets): convert widget style prop keys from camel to dash case

Open TylerMatteo opened this issue 1 year ago • 8 comments

Closes #8990

Change List

  • Update CompassWidget, ZoomWidget, and FullscreenWidget with regex to convert camel cased style keys to their dash cased equivalent.

I settled on replace(/([a-z])([A-Z])/g, '$1-$2') as the regex replacement (credit) but I'm open to suggestions if there is a better approach.

TylerMatteo avatar Jun 29 '24 17:06 TylerMatteo

Coverage Status

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

coveralls avatar Jul 04 '24 07:07 coveralls

@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...

Pessimistress avatar Jul 04 '24 19:07 Pessimistress

@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.

chrisgervang avatar Jul 07 '24 17:07 chrisgervang

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

TylerMatteo avatar Jul 09 '24 17:07 TylerMatteo

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

chrisgervang avatar Jul 09 '24 17:07 chrisgervang

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

Pessimistress avatar Jul 14 '24 17:07 Pessimistress

@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.

TylerMatteo avatar Jul 28 '24 16:07 TylerMatteo

Coverage Status

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

coveralls avatar Aug 13 '24 03:08 coveralls