kgiszewski / Archetype

Archetype is an Umbraco 7 property editor that wraps other installed property editors.
https://github.com/kgiszewski/ArchetypeManual
MIT License
89 stars 54 forks source link

Dollar Signs (Unintentionally?) Collapsing in Pairs? #387

Closed Nicholas-Westby closed 7 years ago

Nicholas-Westby commented 7 years ago

Somebody editing the content for the main navigation (built with Archetype) on a site we're building noticed this:

dollars

It seems that the Archetype header is combining pairs of dollar signs. So, if you have 2, it becomes 1. If you have 3, it becomes 2. If you have 4, it becomes 2.

Here's how the Archetype fieldset is configured:

configuration

My guess is that this is some obscure Archetype feature misbehaving or that maybe this is related to however you are replacing strings with Angular (i.e., perhaps Angular combines dollar signs).

Archetype 1.13.1.

kgiszewski commented 7 years ago

That's not intentional and strange :)

Does escaping it help at all?

Nicholas-Westby commented 7 years ago

Not entirely sure what you mean, but I gave this a try and it didn't help (other than the backslash separating the dollar signs, and so preventing the combining):

escaped

kgiszewski commented 7 years ago

Yep that's what I meant and it's a head-scratcher.

kgiszewski commented 7 years ago

Could you add quotes around then whole expression (sorry I can't think of the reason)?

kjac commented 7 years ago

Ah glorious JS. It's just one of them things... dollar signs have a special meaning in string replacement. And when we calculate the fieldset template, we do indeed use string replacement (and rightfully so). This line is the culprit.

But.. it's "just how JS works", I'm afraid. Try this in your Chrome JS console:

"i am a string".replace("am", "$$$$$")

You'd expect the result to be "i $$$$$ a string", but noooo... the result is "i $$$ a string"

image

Nicholas-Westby commented 7 years ago

Ha, awesome. Never knew that. According to this, you are correct: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace

replacement

Maybe we should first replace all dollar signs with double dollar signs (effectively escaping them) before calling replace?

kjac commented 7 years ago

@Nicholas-Westby that was my first thought too... but I'm afraid it might mess up someone's fancy angular label template, or cause other equally unintended problems.

kgiszewski commented 7 years ago

@Nicholas-Westby this might be more trouble that it's worth to fix. Would using ####-##### suit your needs?

Curious if adding the extra $'s are sufficient as well.

And if you've never seen this video below, you're missing out on life.

https://www.destroyallsoftware.com/talks/wat

kjac commented 7 years ago

@Nicholas-Westby if you really must, you can create your own label template that escapes the dollar signs. A bit annoying, yes, but... I do agree with @kgiszewski, fixing it might lead to a lot of other issues.

Nicholas-Westby commented 7 years ago

I'll probably just ignore the issue. Only happens one a small part of the CMS (back office cosmetic is not on my priority list).

However, I'd be open to submitting a pull request if you were open to it. In my case, it would not be acceptable to add extra dollar signs. And they do need to be dollar signs (they are shown in a navigation menu to represent price ranges), so can't use the pound symbol.

I'm thinking that anybody actually intending to use this would be very unlikely. And even if there are, I'd consider it akin to this: https://xkcd.com/1172/

My perspective is that "what it says it will do on the tin" does not match what it does (i.e., the dollar sign functionality seems like an unintentional side effect rather than something intended and useful). My guess is it causes problems much more often than it solves issues.

I'm OK with whatever you would prefer, but this seems to me to be clearly a bug that ought to be fixed. The fix would be to simply double every dollar sign before calling replace.

And if anybody actually is relying on the dollar sign special behavior:

watman

(Sad Watman)

kjac commented 7 years ago

Hey @Nicholas-Westby,

I'm mostly concerned with people using stuff like $scope and $filter in their label templates... not people relying on this (awesome) JS feature.

Nicholas-Westby commented 7 years ago

@kjac Not sure I follow. I don't see how escaping dollar signs would affect $scope or $filter if it were done just before this line:

replace

Do you mind going into a bit more detail?

To clarify, I'm thinking of simply doubling dollar signs in templateLabelValue to prevent the issue.

kjac commented 7 years ago

This is fixed by #406 - the issue will be closed when the next release is ready.

kjac commented 7 years ago

Fixed in v1.14.1