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 issue with ':' in mustache block quoted text #3988

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

dbolack-ab
Copy link
Collaborator

This fixes the issue with CSS Attribute assignment strings containing : in mustache blocks

Description

This updates the regex to permit : in the right side of a CSS attribute/property or HTML attribute assignment.

Related Issues or Discussions

QA Instructions, Screenshots, Recordings

{{footer,--chapterName:"'1:How-to'"}}

Should now generate

<span class="inline-block footer" style="--chapterName:‘1:How-to’;”></span>

Instead of

<span class="inline-block footer" style="--chapterName:;">“‘1:How-to’”</span>

etc. Should work for all four mustache situations.

Reviewer Checklist

*Reviewers, refer to this list when testing features, or suggest new items *

  • Verify new features are functional
    • Verify with each mustache type.
  • Does this require new tests?

such as style attribute assignments and attribute values
@dbolack-ab dbolack-ab self-assigned this Jan 14, 2025
@dbolack-ab dbolack-ab added tweak Small, non-breaking change ❌ Missing from V3 Was planned for V3 but still missing 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Jan 14, 2025
@ericscheid
Copy link
Collaborator

ericscheid commented Jan 15, 2025

A very similar regex issue occurs with {{footer,--chapterName:"'1=How-to'"}}.

Hmm .. testing the regex in regex101 suggests it should work already, except it doesn't.

https://regex101.com/r/9gEWyC/1

@dbolack-ab
Copy link
Collaborator Author

A very similar regex issue occurs with {{footer,--chapterName:"'1=How-to'"}}.

Hmm .. testing the regex in regex101 suggests it should work already, except it doesn't.

https://regex101.com/r/9gEWyC/1

Yeah, I didn't test those because I wasn't trying to solve for them. :)

Let me see what I can find.

Allows users to not need '\"\'' and '\'\"' to wrap values. double quotes may be used alone. Nested quotes is still supported and translated to single quotes in assignment.
@5e-Cleric
Copy link
Member

this fix is in good place, just tested and it works, should go for a full testing to make sure we don't break anything else

@5e-Cleric
Copy link
Member

Consider this will have to merge with #4004

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3988 February 2, 2025 21:18 Inactive
@@ -828,7 +828,7 @@ const processStyleTags = (string)=>{

const id = _.remove(tags, (tag)=>tag.startsWith('#')).map((tag)=>tag.slice(1))[0] || null;
const classes = _.remove(tags, (tag)=>(!tag.includes(':')) && (!tag.includes('='))).join(' ') || null;
const attributes = _.remove(tags, (tag)=>(tag.includes('='))).map((tag)=>tag.replace(/="?([^"]*)"?/g, '="$1"'))
const attributes = _.remove(tags, (tag)=>(tag.includes('=')) && (tag.slice(0, tag.indexOf(':')).indexOf('=') > 0)).map((tag)=>tag.replace(/="?([^"]*)"?/g, '="$1"'))
Copy link
Member

@calculuschild calculuschild Feb 20, 2025

Choose a reason for hiding this comment

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

Can you clarify what this added check is doing? I don't quite follow what case this is checking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is debris I missed from a different fix attempt. I can't see any sane reason for it not to be true if the previous test is true.

Copy link
Collaborator Author

@dbolack-ab dbolack-ab Feb 21, 2025

Choose a reason for hiding this comment

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

Oh, figured it out. It's incorrectly constructed - I must have tested for it badly. It is meant to filter out CSS property assignments that have an = in the string.

IE: Make sure that --myVar:"This is an = sign" isn't treated as an HTML attribute.

@dbolack-ab
Copy link
Collaborator Author

There is a hard regression in here I am attempting to locate.

@calculuschild
Copy link
Member

@dbolack-ab Ping me when you are ready for me to merge and I will try to do it today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❌ Missing from V3 Was planned for V3 but still missing 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed tweak Small, non-breaking change
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

CSS property setting syntax does not permit the use of a colon (:) in the assigned value.
5 participants