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(dropdown): values are incorrect if they contain some special chars #1210

Merged
merged 4 commits into from
Dec 22, 2019

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Dec 6, 2019

Description

Dropdown values which are supposed to be encoded into htmlentities like < were wrongly encoded whenever the values changed. Especially when working with mulitple selection dropdowns.
Reason for this was the overseen fact that an ampersand & was also encoded into &. As htmlentities are in general encoded by starting with an ampersand aswell this was totally messing up the value string.

As the escape routine was also implemented in other modules, this PR fixes the logic there as well

Testcase

Broken

https://jsfiddle.net/4cqtnf0d/

Fixed

https://jsfiddle.net/yfgwph04/

Screenshot

Before After
image image

Closes

#1207

@lubber-de lubber-de added type/bug Any issue which is a bug or PR which fixes a bug lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews labels Dec 6, 2019
@lubber-de lubber-de added this to the 2.8.3 milestone Dec 6, 2019
y0hami
y0hami previously approved these changes Dec 6, 2019
Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

@exoego
Copy link
Contributor

exoego commented Dec 6, 2019

XML-like values (e.g. <abc>) are not rendered in dropdown both in current and this PR.
Is this as-designed for some reason?
https://jsfiddle.net/2cp5gL7o/

Since < and > are rendered in dropdown, I think <xml-like> should be rendered too.
This might be trickier, so could be addressed in later.

@lubber-de lubber-de added the state/on-hold Issues and pull requests which are on hold for any reason label Dec 7, 2019
@lubber-de
Copy link
Member Author

lubber-de commented Dec 7, 2019

Since < and > are rendered in dropdown, I think <xml-like> should be rendered too.
This might be trickier, so could be addressed in later.

@exoego <xml> is valid html, thus it's parsed and used as such by default. If you want to display such html code, it works by using preserveHTML:false
See https://jsfiddle.net/uLejd364/1/
image

@lubber-de
Copy link
Member Author

@hammy2899 I had to fix the remove label selection as @GammaGames mentioned , so please review again. I updated the jsfiddle in the description accordingly https://jsfiddle.net/uLejd364/

@lubber-de lubber-de removed the state/on-hold Issues and pull requests which are on hold for any reason label Dec 7, 2019
exoego
exoego previously approved these changes Dec 7, 2019
Copy link
Contributor

@exoego exoego left a comment

Choose a reason for hiding this comment

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

LGTM
Now I knew preserverHTMl option.

@lubber-de
Copy link
Member Author

@exoego @hammy2899 Please review again, i also fixed the situation when allowAdditions: true was set (entries came up twice then... 🙄 )
See here:

Broken

https://jsfiddle.net/wckbtaey/

Fixed

https://jsfiddle.net/wckbtaey/1/

@y0hami y0hami requested review from exoego and y0hami December 8, 2019 17:19
Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@exoego exoego left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami y0hami changed the title [Dropdown] Values are parsed incorrectly when they contain <, ≤, etc fix(dropdown): values are incorrect if they contain some special chars Dec 22, 2019
@y0hami y0hami merged commit fa50976 into fomantic:develop Dec 22, 2019
@lubber-de lubber-de deleted the fix/1207/ampersand_escaping branch December 22, 2019 15:46
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants