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 liquid template copy component 1337 #1348

Merged

Conversation

JayKay24
Copy link
Contributor

@JayKay24 JayKay24 commented Aug 6, 2023

Description

This PR fixes #1337

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

@netlify
Copy link

netlify bot commented Aug 6, 2023

Deploy Preview for mesheryio-preview ready!

Name Link
🔨 Latest commit a29620c
🔍 Latest deploy log https://app.netlify.com/sites/mesheryio-preview/deploys/6536a6eaa7f74f00085603a2
😎 Deploy Preview https://deploy-preview-1348--mesheryio-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Ghat0tkach
Copy link
Member

hey @JayKay24 ,

Let's discuss it on the websites call.
Please add this as an agenda item in the meeting minutes, if you would :)

https://docs.google.com/document/d/1XczAHXVe2FIWPqiF57ospJ43zw5cZQ7ui8mn39v5EvA/edit#heading=h.lohhtewfwima

Copy link
Contributor

@thisiskaransgit thisiskaransgit left a comment

Choose a reason for hiding this comment

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

@JayKay24, thanks much, please take a look into the footer.html to make sure there's no overlapping script.

@JayKay24
Copy link
Contributor Author

JayKay24 commented Aug 9, 2023

@thisiskaransgit Thanks for your review. The code in footer.html only has the javascript function(resetCopyText) and dependency(clipboard.min.js) needed to copy a piece of text. It is only defined in that file. I abstracted away the repeated HTML into a copy-to-clipboard.html template that still references the resetCopyText defined in footer.html.

Copy link
Contributor

@thisiskaransgit thisiskaransgit left a comment

Choose a reason for hiding this comment

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

We are good to then, thanks @JayKay24 👍🏼

@JayKay24
Copy link
Contributor Author

I've updated the branch to keep it mergable but will need a re-review @thisiskaransgit

Copy link
Contributor

@thisiskaransgit thisiskaransgit left a comment

Choose a reason for hiding this comment

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

@JayKay24, use of CDN, we need to replace it with a locally available file, otherwise the payload will not be improved. You can reference the implementation in docs.meshery.io if required. I overlooked it initially but we need this update

@JayKay24 JayKay24 force-pushed the fix-liquid-template-copy-component-1337 branch from 6009168 to 92ca9f6 Compare August 20, 2023 10:11
@JayKay24
Copy link
Contributor Author

@thisiskaransgit does that mean that I integrate npm into the project? This would allow me to use the clipboard library currently loaded via CDN as a local npm module.

@thisiskaransgit
Copy link
Contributor

@thisiskaransgit does that mean that I integrate npm into the project? This would allow me to use the clipboard library currently loaded via CDN as a local npm module.

@JayKay24 declare the javascript logic in the code. the desired behavior should be that the class element can use the locally hosted code and not third-party module, which will a) reduce load b) filter the unused code that comes along c) opt out of depending on other services. This is what I intend to update for other sections too actually.

take a look at here for reference: https://github.com/meshery/meshery/blob/master/docs/assets/js/main.js

@Chadha93
Copy link
Member

@thisiskaransgit does that mean that I integrate npm into the project? This would allow me to use the clipboard library currently loaded via CDN as a local npm module.

@JayKay24 declare the javascript logic in the code. the desired behavior should be that the class element can use the locally hosted code and not third-party module, which will a) reduce load b) filter the unused code that comes along c) opt out of depending on other services. This is what I intend to update for other sections too actually.

take a look at here for reference: meshery/meshery@master/docs/assets/js/main.js

@JayKay24, does this helps?

@JayKay24
Copy link
Contributor Author

@Chadha93 i haven't had a chance to dive deep into the solution but from what I can tell is that I will wrap the clipboard logic inside an IIFE. Will look to address this tomorrow. I'll post any questions I have in this thread.

@JayKay24
Copy link
Contributor Author

@thisiskaransgit The example you gave me also references the clipboard.min.js library via CDN in this file -> https://github.com/meshery/meshery/blob/master/docs/_layouts/default.html.

I don't understand what you mean when you say the payload will be improved. Which payload is this? Do you mean that what we want is to avoid the extra HTTP request of downloading the clipboard.min.js dependency on page load in order to achieve a faster loading rate by having the browser parse a local .js file with clipboard logic?
If this is the case, there's a way to use the dependency locally by downloading and extracting the zip file if we're not into package management. But if we do it this way, we won't have a way to keep it up to date easily.
The other option is to integrate the project with npm and reference clipboard.js as a node module. This way when the project is built, the dependency will be included on page load without any extra HTTP request to the CDN.
Please help me to clarify this matter. Sorry If I'm not understanding it as quickly.
@Chadha93 please let me know your insights on this. What could I be missing?

@stale
Copy link

stale bot commented Sep 27, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the issue/stale Issue has not had any activity for an extended period of time label Sep 27, 2023
@stale
Copy link

stale bot commented Oct 5, 2023

This issue is being automatically closed due to inactivity. However, you may choose to reopen this issue.

@stale stale bot closed this Oct 5, 2023
@vishalvivekm vishalvivekm removed the issue/stale Issue has not had any activity for an extended period of time label Oct 15, 2023
@vishalvivekm vishalvivekm reopened this Oct 15, 2023
Signed-off-by: Vivek Vishal <[email protected]>
Signed-off-by: Vivek Vishal <[email protected]>
Instead of assigning values to variables, here I'm directly passing them to parameters in `include` tag

Signed-off-by: Vivek Vishal <[email protected]>
Signed-off-by: Vivek Vishal <[email protected]>
Copy link
Member

@vishalvivekm vishalvivekm left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Thanks @JayKay24 🚀

@vishalvivekm vishalvivekm merged commit 716e2ae into meshery:master Oct 23, 2023
6 checks passed
@welcome
Copy link

welcome bot commented Oct 23, 2023

Thanks for your contribution to the Layer5 and Meshery community! 🎉

Meshery Logo
        ⭐ Please leave a star on the project. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Liquid Template for Code Copy Component
5 participants