search

liferay / liferay-frontend-projects

A monorepo containing assorted Frontend Infrastructure Team projects
Other
66 stars 67 forks source link

LPS-188042-master #1150

Closed Marina-Gouveia closed 12 months ago

antonio-ortega commented 12 months ago

Hey @Marina-Gouveia ,

At a first glance, It seems a bit strange to me we need to allow Japanese characters in a regex since it's something we don't need anywhere else in Liferay or AlloyUI.

Reading the issue, it seems pretty tied to Workflow, so it would help us a lot to review this PR if you can give us some brief explanation about why this change is needed in AlloyUI.

Thanks!

Marina-Gouveia commented 12 months ago

HI @antonio-ortega If possible Could you look at this PTR? We explain better why we need to change the regex https://liferay.atlassian.net/browse/PTR-4069

antonio-ortega commented 12 months ago

Hey @Marina-Gouveia ,

I've been doing a deeper analysis of this case and I could see the reason why we messed it up after removing that "1" from the node name is because we have nodeId's duplicated.

And we have them duplicated because we build them in the way you see in the code you are modifying, so any non-alphanumeric character is removed, and because of that, if you inspect your DOM you'll see a few data-nodeid="diagramNode_field_____".

The idea to remove any non-alphanumeric character was introduced in this commit as part of AUI-461.

As of today, it's not illegal to use non-alphanumeric characters to build HTML ids although it is discouraged as you can read here:

Technically, the value for an id attribute may contain any character, except whitespace characters. However, to avoid inadvertent errors, only ASCII letters, digits, '_', and '-' should be used, and the value for an id attribute should start with a letter.

Apart from that, there is another reason to try to find a different approach: With your solution you would be fixing the case for CJK languages, but what if we introduce a Node name in Arabic? or Russian? We would fail into the same error.

If the node name introduced in the user interface can be in any language, then I think the solution would be to modify the way how we generate that nodeId to avoid using the nodeName and instead of that use some number generated randomly, for example. However I don't know how that might impact in Workflow code, so you'd need thoroughly test it and check whether any change is required in that part.

Thanks.

Regards.