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

docs: add a StackBlitz "Try It" button in code examples #35644

Merged
merged 9 commits into from
Mar 9, 2022

Conversation

PuruVJ
Copy link
Contributor

@PuruVJ PuruVJ commented Jan 3, 2022

Based on #35188, this PR adds the "Edit" button on code examples, clicking which will open up stackblitz editor in new tab with the code example in mind.

Here's a video:

CleanShot.2022-01-03.at.00.29.35.mp4

Preview: https://deploy-preview-35644--twbs-bootstrap.netlify.app/

@PuruVJ PuruVJ changed the title Feat/edit-button [NEW] "Edit" button on code examples Jan 3, 2022
@PuruVJ PuruVJ marked this pull request as ready for review January 3, 2022 10:46
@PuruVJ PuruVJ requested review from a team as code owners January 3, 2022 10:46
@XhmikosR XhmikosR marked this pull request as draft January 3, 2022 13:00
@GeoSot GeoSot added the docs label Jan 3, 2022
@PuruVJ
Copy link
Contributor Author

PuruVJ commented Jan 4, 2022

Can anyone review this?

@PuruVJ PuruVJ marked this pull request as ready for review January 4, 2022 08:34
@GeoSot
Copy link
Member

GeoSot commented Jan 4, 2022

Hello @PuruVJ .

Thank you for your contribution. We may see that we are only a few people here for reviews, so please have patience

I did a check on the netlify preview and got this (on several examples)
image

@PuruVJ
Copy link
Contributor Author

PuruVJ commented Jan 4, 2022

That's strange. I'll look into it

@PuruVJ
Copy link
Contributor Author

PuruVJ commented Jan 4, 2022

@GeoSot can you try disabling your adblocker/Brave shields on stackblitz?

Cuz it is working perfectly for me 🤔

@PuruVJ PuruVJ changed the title [NEW] "Edit" button on code examples [NEW] "⚡️Try It" button on code examples Jan 5, 2022
@GeoSot
Copy link
Member

GeoSot commented Jan 5, 2022

To be honest, I love it 😍

Wish the rest of the team have the same opinion
@twbs/css-review
@twbs/js-review

@GeoSot can you try disabling your adblocker/Brave shields on stackblitz?
Cuz it is working perfectly for me thinking

It seems related to firefox on both linux & win10 systems. Edge & chrome are fine

@XhmikosR
Copy link
Member

XhmikosR commented Jan 5, 2022

I don't like the fact we load this in all pages and it definitely needs some more tweaks...

@XhmikosR XhmikosR changed the title [NEW] "⚡️Try It" button on code examples docs: add a "Try It" button in code examples Jan 5, 2022
@XhmikosR XhmikosR changed the title docs: add a "Try It" button in code examples docs: add a StackBlitz "Try It" button in code examples Jan 5, 2022
@XhmikosR
Copy link
Member

XhmikosR commented Jan 5, 2022

Also, the fact that this doesn't work with Firefox is a huge downside to the point we shouldn't land it unless fixed.

@PuruVJ
Copy link
Contributor Author

PuruVJ commented Jan 5, 2022

@XhmikosR True, it makes no sense if it can't work on the major browsers. I'll look into it

@PuruVJ
Copy link
Contributor Author

PuruVJ commented Jan 5, 2022

Can you recommend a place I can put the open in stackblitz logic such that it will load only in the right pages?

And, even if we aren't able to selectively load it, the SDK is only 2.1kb to load, so for now it won't be as critical of an issue
CleanShot 2022-01-05 at 18 04 52@2x

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things to improve and consider. The feature looks like a good idea IMHO. :)

site/assets/js/application.js Outdated Show resolved Hide resolved
site/assets/scss/_clipboard-js.scss Show resolved Hide resolved
site/assets/scss/_clipboard-js.scss Show resolved Hide resolved
site/layouts/partials/scripts.html Outdated Show resolved Hide resolved
@PuruVJ PuruVJ requested review from ffoodd and GeoSot January 5, 2022 18:11
@XhmikosR
Copy link
Member

XhmikosR commented Jan 5, 2022

Can you recommend a place I can put the open in stackblitz logic such that it will load only in the right pages?

And, even if we aren't able to selectively load it, the SDK is only 2.1kb to load, so for now it won't be as critical of an issue

It's not a matter of size only. JS affects execution time and everything. I find no reason to include it everywhere.

Also, we should load the script from our server which is where we are loading all assets from.

@XhmikosR XhmikosR marked this pull request as draft January 5, 2022 18:24
@PuruVJ
Copy link
Contributor Author

PuruVJ commented Jan 5, 2022

Alright. So can you help me with where to put the script? Or am I on my own for that part?

I have a way in mind to selectively load but it's horribly hacky and I wanna get suggestions before from people who know the codebase better than me 🙂

@XhmikosR
Copy link
Member

XhmikosR commented Jan 5, 2022

Please don't mark this as ready. It's broken right now.

@XhmikosR
Copy link
Member

XhmikosR commented Mar 2, 2022

The title is still wrong AFAICT.

@PuruVJ
Copy link
Contributor Author

PuruVJ commented Mar 2, 2022

The title is good now. I just checked on my own. Can you share ss of what yur seeing?

@XhmikosR
Copy link
Member

XhmikosR commented Mar 2, 2022

Copy to Clipboard is shown on the Try it button as one can see in the preview.

@PuruVJ
Copy link
Contributor Author

PuruVJ commented Mar 2, 2022

I am seeing this locally. Can you share the netlify preview?

CleanShot.2022-03-02.at.14.34.18.mp4

@XhmikosR
Copy link
Member

XhmikosR commented Mar 2, 2022

@PuruVJ
Copy link
Contributor Author

PuruVJ commented Mar 2, 2022

Oof my bad. Just fixed now

@XhmikosR
Copy link
Member

XhmikosR commented Mar 2, 2022

Cool, it works now. One last thing I notice is that the Try button's tooltip is still shown even after closing the StackBlitz tab.

@XhmikosR
Copy link
Member

XhmikosR commented Mar 2, 2022

...and for some reason the HTML tab is empty on Edge Chromium, and I get nothing loaded on Firefox. These 2 issues are probably not related to this PR, something looks broken on their side.

@GeoSot
Copy link
Member

GeoSot commented Mar 2, 2022

One last thing I notice is that the Try button's tooltip is still shown even after closing the StackBlitz tab.

I tried to handle it, using a fabulous timeout. It closes the tip, opens the new window... and we are fine.
However when we close the stackblitz tab, and return to BS tab we have left of, the mouse is still on the same spot, so js gets the hover event and re-shows the tooltip.
I don't think we can do a lot on this

Hope you don't mind, I pushed two minor changes. One typo and the other forces the stackblitz editor to open the file

In short, if we do not want any other optimizations, I think we are good to go 😮

(The only optimization I can think of, is to use the stackblitz post api , instead of SDK (only 2kb), but I suppose we can leave this way)

@PuruVJ
Copy link
Contributor Author

PuruVJ commented Mar 8, 2022

Hi, might I ask what is the blocker in merging this 🙂

@XhmikosR
Copy link
Member

XhmikosR commented Mar 9, 2022

I'd still like the Firefox Private Window to be fixed somehow upstream, but for now I don't think there's anything we can do here :/

We can revisit loading and stuff later if needed.

@PuruVJ Thanks for your patience and your contribution!

@XhmikosR XhmikosR merged commit 645f955 into twbs:main Mar 9, 2022
@PuruVJ PuruVJ deleted the feat/edit-button branch March 9, 2022 15:58
GeoSot added a commit that referenced this pull request Mar 11, 2022
XhmikosR pushed a commit that referenced this pull request Mar 11, 2022
XhmikosR pushed a commit that referenced this pull request Mar 11, 2022
@mdo mdo mentioned this pull request Apr 12, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants