-
-
Notifications
You must be signed in to change notification settings - Fork 78.8k
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 StackBlitz examples by embedding snippets.js when needed #36352
Fix StackBlitz examples by embedding snippets.js when needed #36352
Conversation
}, | ||
title: 'Bootstrap Example', | ||
description: `Official example from ${window.location.href}`, | ||
template: 'html', | ||
template: jsSnippet ? 'javascript' : 'html', |
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.
isn't it ok just use javascript
template?
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've used the appropriate template in function of the context but javascript
would work in both cases yes if we want to simplify. But in this case we would need to create an empty index.js
file. The advantage with the html
template is that this extra empty index.js
file isn't mandatory.
However in https://developer.stackblitz.com/docs/platform/javascript-sdk/#sdkopenprojectproject-opts I don't see any tags
options. Since html
(as template) is not mentioned neither maybe I don't have the latest doc reference 🤷
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.
we may (at least for start) leave it with javascript template, and include examples.js
by default
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.
Made the exercise in this PR to include js_snippet="true"
only when needed in {{< example >}}
. Only few examples really need a javascript
template just to show the impact.
What's requiring us to remove those examples using |
Considering Popovers - Live demo. In the Markdown, it is declared this way: {{< example >}}
<button type="button" class="btn btn-lg btn-danger" data-bs-toggle="popover" title="Popover title" data-bs-content="And here's some amazing content. It's very engaging. Right?">Click to toggle popover</button>
{{< /example >}} At the load of this page Bootstrap JS will "consume" and delete this <button type="button" class="btn btn-lg btn-danger" data-bs-toggle="popover" data-bs-content="And here's some amazing content. It's very engaging. Right?">Click to toggle popover</button> This is this code that will be used by StackBlitz when we click on the "Edit on StackBlitz" button. The title information is lost in the way so the example won't work; at least it won't work entirely as you can see when you click on it (the popover doesn't have any header/title). Based on this observation and the discussion we had with GeoSot we could hide the "Edit on StackBlitz button" for those examples. If we don't mind modifying the documentation a bit we could rather use in some of them In this PR if you don't mind I would propose to not to tackle this issue in order to move on this StackBlitz topic step-by-step:
|
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.
Sounds good! Let me know when we're good to go here :).
cf2e928
to
b3e440a
Compare
Resolved the conflicts. Ready on my side. |
b3e440a
to
4c657cc
Compare
Alternate version of #36127
Linked to tracking PR #36091
This PR is an alternate version of #36127 based on its comments.
The philosophy here is to indicate when needed that a given example needs to embed the extra file
snippets.js
in StackBlitz:data-js-snippet="true"
we create a JS StackBlitz environment with this fileIn order for the user to understand the logic here (that all the JS file content is not needed) IMO
snippets.js
should be heavily commented and crystal clear)At the moment this PR only show the architecture changes. We already know that the popover example should be at the end hidden.
Extra things modified:
.bd-example
Thoughts?
Remaining tasks
snippets.js
data-js-snippet="true"
everywhere in the docs when needed (see [WIP] Hide or fix StackBlitz examples #36091 to have the list)Tasks for other PRs
title
removed by JS in the DOM. They must be hidden.Live previews
Alerts
Checks & Radios
Modal
Popovers
title
won't work as it is removed automatically by JSToasts
Tooltips
title
won't work as it is removed automatically by JS.title
won't work as it is removed automatically by JS.Non-regression example (just
html
)snippets.js
is not embedded in StackBlitz; there's noindex.js
created since no extra JS is needed.