-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Angular: Fix handling of line breaks with multiple selectors #14313
Angular: Fix handling of line breaks with multiple selectors #14313
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Just a suggestion to try to improve code readability
You did well to add tests to increase confidence in these not very digest regex
inputs: string, | ||
outputs: string | ||
) => { | ||
/* eslint-disable no-return-assign, no-param-reassign */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the original one not very readable and I propose another solution that uses a reduce. what do you think ?
const templateReplacers: [
string | RegExp,
string | ((substring: string, ...args: any[]) => string)
][] = [
[/(^\..+)/, 'div$1'],
[/(^\[.+?])/, 'div$1'],
[/([\w[\]]+)(\s*,[\w\s-[\],]+)+/, `$1`],
[/#([\w-]+)/, ` id="$1"`],
[/((\.[\w-]+)+)/, (_, c) => ` class="${c.split`.`.join` `.trim()}"`],
[/(\[.+?])/g, (_, a) => ` ${a.slice(1, -1)}`],
[/([\S]+)(.*)/, `<$1$2${inputs}${outputs}>${innerTemplate}</$1>`],
];
return templateReplacers.reduce(
(prevSelector, [searchValue, replacer]) => prevSelector.replace(searchValue, replacer as any),
selector
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. reduce expresses the intent better than map. Let me do this
@@ -216,3 +216,220 @@ Object { | |||
], | |||
} | |||
`; | |||
|
|||
exports[`cra-ts-essentials manager production mode 1`] = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I don't understand this change. Is it necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this either 😄 I only changed the template file and tests. I think yarn test -u might be the culprit. I'll try remove this file with my next commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please assist with the latest commit. I tried to revert this file hoping it would be removed from the PR but the new commit shows the deleted lines. Is it as simple as using "Delete file" from the menu in "Files changed" to remove it from the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. I see it's been removed from Files changed
@@ -216,220 +216,3 @@ Object { | |||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be merged like this but I think it would be better to remove this file from the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ;)
@shilman Do you think this could be merged soon? |
@dexster yes, possibly today. we've been in a merge freeze while the latest release is stabilizing, but i'm planning on lifting that and commencing 6.3 alphas. after it's released and tested on alpha, i can patch it back to 6.2.x if @ThibaudAV gives the thumbs up. |
I'm new to SB, but I think the regex is not handling attribute selectors and class selectors, like:
When I remove the classes, it works. When I do a single class selector, it works. etc. EDIT: Bug ticket created: #15975 |
Issue: #14079
What I did
Previous PR was faulty and missing some of the recommendations by @ThibaudAV. This commit includes them as well as extra checks for line breaks in multiple selectors.
How to test
Tests added for all scenarios