web: Improve UX for copyCode animation #48958
Conversation
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
|
|
k8s-ci-robot
added
the
cncf-cla: no
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/M
|
The failed build looks like an internal error (nil pointer error) within hugo. Build log excerpt4:50:46 AM: Failed during stage 'building site': Build script returned non-zero exit code: 2 (https://ntl.fyi/exit-code-2) 4:50:46 AM: ERROR render of "page" failed: "/opt/build/repo/layouts/docs/baseof.html:59:7": execute of template failed: template: docs/single.html:59:7: executing "docs/single.html" at <partial "scripts.html" .>: error calling partial: "/opt/build/repo/layouts/partials/scripts.html:55:3": execute of template failed: template: partials/scripts.html:55:3: executing "partials/scripts.html" at <partial "hooks/body-end.html" .>: error calling partial: "/opt/build/repo/layouts/partials/hooks/body-end.html:11:29": execute of template failed: template: partials/hooks/body-end.html:11:29: executing "partials/hooks/body-end.html" at <$toastrCss.RelPermalink>: nil pointer evaluating resource.Resource.RelPermalink 4:50:46 AM: Total in 39071 ms |
Oh, I see... |
apoorvapendse
force-pushed
the
main
branch
2 times, most recently
from
3dc1438 to
8fbfee2
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
apoorvapendse
force-pushed
the
main
branch
2 times, most recently
from
e29c87a to
c51f0c7
Compare
|
LGTM if #48958 (comment) is fixed |
layouts/partials/hooks/body-end.html
Outdated
| <script src="https://cdnjs.cloudflare.com/ajax/libs/toastr.js/latest/toastr.min.js"></script> | ||
| <script src="https://cdnjs.cloudflare.com/ajax/libs/toastr.js/latest/toastr.min.js"></script> |
There was a problem hiding this comment.
Why latest for a preview but the committed code for production?
Better to do one of:
- use
js/toastr-2.1.4.min.jseither way - load from the CDN either way, pin to a specific release, and hard code the expected value of
integrity
There was a problem hiding this comment.
Yeah sure 👍
Actually that url isn't maintained anymore
I guess I'll go with the local file for both.
There was a problem hiding this comment.
So, do we need an if block for this case at all?
Since we are loading the same files in both the cases?
data/i18n/en/en.toml
Outdated
There was a problem hiding this comment.
Consider moving the newly added entries in this "en.toml" file based on alphabetical order to align with the rest of the entries.
There was a problem hiding this comment.
The order for blog and banner seem mismatched, should I fix that too?
This commit introduces a new toast animation instead of the previous modal that would have to be manually closed. The new toast is less intrusive to the user's workflow and closes by itself. Fixes issue: 48950. Signed-off-by: Apoorva Pendse <apoorvavpendse@gmail.com>
|
Hey, I've updated the PR as per the feedback. |
|
@sftim, any other changes that are required for this PR to be considered? |
|
Cool /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 5ab87dd5e2d52cf0c12c94b96e9061b6a93f7512
|
|
It might be a few weeks before we can get this approved, I'm afraid. |
There was a problem hiding this comment.
Hi all. Docsy supports a click-to-copy button functionality for all code blocks, as @sftim noticed in #48812 (comment).
For example, here's the tooltip on hover:
This is after the button is clicked:
In the spirit of ongoing Align with upstream Docsy #41171, if we're to spend some effort improving on #48950, would it not be better to alight with Docsy's offering?
Docsy's click-to-copy works out-of-the-box for all code blocks, is compatible with dark mode, etc.
(Btw, official Docsy support for click to copy started in Docsy 0.7, but the feature elements are a part of 0.5.)
/cc @nate-double-u
|
I'm OK to merge this and then change again once we have newer Docsy. We've had click-to-copy since before Docsy; improving what we have is useful, even if we eventually converge. |
Thanks for the details @chalin. |
Sure 👍 |
| @@ -6,29 +6,54 @@ | |||
| {{ end }} | |||
| {{/* copy-and-paste helper for codenew shortcode */}} | |||
| {{- if or (.HasShortcode "code_sample") (.HasShortcode "code") (.HasShortcode "codenew") -}} | |||
| {{- $toastrJs := resources.Get "js/toastr-2.1.4.min.js" | minify | fingerprint -}} | |||
There was a problem hiding this comment.
BTW: we'd typically only minify the code in production builds.
There was a problem hiding this comment.
okay I'll remove the pipe.
There was a problem hiding this comment.
@sftim, but js file on the server in already minified, so what should be the next step?
There was a problem hiding this comment.
Should I add a non minified version just for development?
Description
This PR introduces a new toast animation instead
of the previous modal that would have to be manually closed.
The new toast is less intrusive to the user's workflow
and closes by itself.
preview
copy_to_clipboard_updated_animation.mp4
Issue
#48950.
Closes: #48950.