-
Notifications
You must be signed in to change notification settings - Fork 12
OEL-4230: Remove spaceless from templates and fix for tests whitespace. #710
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
Conversation
|
🚀 Deployed on https://preview-710--oelibrary.netlify.app |
| {% macro render(items, pills, tabs, tabs_content, vertical, full_width, alignment, id, nav, navbar, attributes) %} | ||
| {% apply spaceless %} | ||
| {% import _self as navigation %} | ||
| {% import _self as navigation %} |
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 think identation is wrong.
tools/test-utils/index.js
Outdated
|
|
||
| const escapeRegExp = (value) => value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| let start = 0; | ||
| let seenLineBreak = false; |
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 think the removals of the spaceless is ok, Drupal core has done this, Im wondering if we should remove spaces from the tests. This is altering the real output, drupal-attribute is ok since will be the expected result when templates go through Drupal, but not sure if spaces/break lines are removed, so this is not what the HTML will be.
Do we want to remove the trim of extra spaces and break lines? If we remove spaceless and tests start to fail that tells me that we are not replacing the functionality. Do we want to keep it as it was?
I've seen we use - or ~to remove extra spaces in some templates, assuming we keep the same functinality maybe we can work around that but may not fit in all scenarios.
fdd1b3f to
a0acc99
Compare
36477a0 to
1584989
Compare
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.
In this file, the side effect of these spaces is real, it causes extra space between text and icon.
We should totally insert - to avoid side effects.
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.
Actually, the problem is not the link template, but the icon template!
The link template already contains plenty of -. But the output from icon needs to be trimmed.
|
We are going for @donquixote fix. It's cleaning up the whitespace better than this pr. |
No description provided.