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 StackBlitz examples by embedding snippets.js when needed #36352

Merged

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented May 14, 2022

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:

  • by default, we create an HTML StackBlitz environment without this file
  • by setting data-js-snippet="true" we create a JS StackBlitz environment with this file

In 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:

  • Gather other classes used in combination with .bd-example

Thoughts?

Remaining tasks

Tasks for other PRs

  • In another PR handle the edge cases for tooltips and popovers examples that won't work anyway because of the title removed by JS in the DOM. They must be hidden.
  • In another PR handle the CSS/SCSS reusable code needed by the examples.

Live previews

Alerts

Checks & Radios

Modal

Popovers

Toasts

  • Placement: toasts are moving but will need corresponding CSS in the future

Tooltips

  • Tooltips on links: should be hidden in a next PR because title won't work as it is removed automatically by JS.
  • Custom tooltips: should be hidden in a next PR because title won't work as it is removed automatically by JS.

Non-regression example (just html)

  • Accordion: snippets.js is not embedded in StackBlitz; there's no index.js created since no extra JS is needed.

@julien-deramond julien-deramond requested a review from a team as a code owner May 14, 2022 09:21
@julien-deramond julien-deramond marked this pull request as draft May 14, 2022 09:21
@GeoSot GeoSot added this to In progress in v5.2.0-stable via automation May 15, 2022
},
title: 'Bootstrap Example',
description: `Official example from ${window.location.href}`,
template: 'html',
template: jsSnippet ? 'javascript' : 'html',
Copy link
Member

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?

Copy link
Member Author

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 🤷

Copy link
Member

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

Copy link
Member Author

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.

@julien-deramond julien-deramond marked this pull request as ready for review May 15, 2022 15:58
@GeoSot GeoSot moved this from In progress to Review in progress in v5.2.0-stable May 18, 2022
@mdo
Copy link
Member

mdo commented May 20, 2022

What's requiring us to remove those examples using title if they work as expected in the docs?

@julien-deramond
Copy link
Member Author

What's requiring us to remove those examples using title if they work as expected in the docs?

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 title to create the popover and display it.
As a result we have in the DOM:

<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 data-bs-title instead of title.


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:

  • Merge this PR when it is considered as good (or the alternative one)
  • Do the same work in another PR to handle the specific CSS code used by some examples
  • Tackle the remaining edge cases one by one in separate PRs like the title for popovers and tooltips to finish all the tasks listed in [Tracking] Fix StackBlitz examples #36391

v5.2.0-stable automation moved this from Review in progress to Reviewer approved May 24, 2022
Copy link
Member

@mdo mdo left a 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 :).

@julien-deramond julien-deramond force-pushed the main-jd-use-js-snippet-file-in-stackblitz branch from cf2e928 to b3e440a Compare May 25, 2022 20:55
@julien-deramond
Copy link
Member Author

Sounds good! Let me know when we're good to go here :).

Resolved the conflicts. Ready on my side.

@mdo mdo force-pushed the main-jd-use-js-snippet-file-in-stackblitz branch from b3e440a to 4c657cc Compare May 26, 2022 21:25
@mdo mdo merged commit 8b85267 into twbs:main May 26, 2022
v5.2.0-stable automation moved this from Reviewer approved to Done May 26, 2022
@XhmikosR XhmikosR added feature and removed feature labels Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.2.0-stable
  
Done
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants