Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
| text = text.replace(/^[\t]*\n/, '').replace(/\n[\t]*$/, ''); | ||
| const templateMatch = text.match(/^[\t]*<template>[\n]*/); | ||
| this._isTemplate = templateMatch && templateMatch.length > 0; | ||
| text = text.replace(/^(\t*\n)*/, '').replace(/(\n\t*)*$/, ''); |
There was a problem hiding this comment.
The regex now handles multiple whitelines, before the code would break if 2 white lines where included in the demos. Not something we should expect but it should not break either
| // fix script whitespace | ||
| text = setIndent(text.replace(/\t/g, ' ')) | ||
| .replace(/<\/script>/g, '\n</script>') | ||
| .replace(/<script>/g, '<script>\n') | ||
| .replace(/<script type="module">/g, '<script type="module">\n') | ||
| .replace(/<script data-demo-hide(.+?)<\/script>/gis, '') | ||
| .split('\n'); | ||
| let scriptIndent = 0; | ||
| lines = lines.map((l) => { | ||
| if (l.indexOf('<script') > -1) { | ||
| scriptIndent = l.match(/^(\s*)/)[0].length; | ||
| return l; | ||
| } else if (l.indexOf('</script>') > -1) { | ||
| const nl = this._repeat(' ', scriptIndent) + l ; | ||
| scriptIndent = 0; | ||
| return nl; | ||
| } else if (scriptIndent && !this._isTemplate) { | ||
| return this._repeat(' ', scriptIndent + 2) + l; | ||
| } else { | ||
| return l; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Before we would look for script tags, and only fix indentation for those, now the same logic is applied to the whole snippet on setIndent. This is needed for when custom lit templates can be set, the function can handle cases where we want to ignore the first line (e.g. a template that starts inline but spans multiple lines html`<some-tag>\n ...) or we want to set an indentation level(e.g. nested templates)
There was a problem hiding this comment.
I remember many years ago when the snippet was originally built, the indentation was really finicky. Not critical, but I wish we had a few tests that covered the indentation/formatting scenarios.
| }); | ||
| } | ||
|
|
||
| _applyAttr(name, value, applyToShadowRoot) { |
There was a problem hiding this comment.
This was meant to handle setting rtl, that feature was removed so this is not needed anymore. This is now handled by _getDemoNodes() and _handleSkeletonChange()
| } | ||
| } | ||
|
|
||
| _repeat(value, times) { |
There was a problem hiding this comment.
This was a guard for string.repeat on IE11
|
I might ask, if there are any existing tests that will fail after these changes. or in other words, are there any tests that need to be updated ? if not, should new tests be added for these changes ? |
Good point, the tests are very barebones, I'll add some |
|
Hmm, looks like a test is timing out in Webkit... |
GZolla
dismissed
dlockhart’s stale review
Still need to figure out the timing issue
…spaceUI/core into gzolla/clean-demo-snippet
| "test": "npm run lint && npm run test:translations && npm run test:unit && npm run test:axe", | ||
| "test:axe": "d2l-test-runner axe --chrome", | ||
| "test:unit": "d2l-test-runner", | ||
| "test:unit": "d2l-test-runner --safari", |
|
🎉 This PR is included in version 3.227.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
GAUD-9549
There is a lot of code on the demo snippet component that is no longer needed: