zulip icon indicating copy to clipboard operation
zulip copied to clipboard

rendered_markdown: Fix code getting copied when visiting custom playground.

Open nimishmedatwal opened this issue 10 months ago • 3 comments

This pr fixes code getting copied even when View in xxx playground is clicked.

Fixes: #29844.

Screenshots and screen captures:

Before After
Animation10 Animation11
Self-review checklist
  • [x] Self-reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • [x] Explains differences from previous plans (e.g., issue description).
  • [x] Highlights technical choices and bugs encountered.
  • [x] Calls out remaining decisions and concerns.
  • [x] Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • [x] Each commit is a coherent idea.
  • [x] Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • [x] Visual appearance of the changes.
  • [x] Responsiveness and internationalization.
  • [x] Strings and tooltips.
  • [x] End-to-end functionality of buttons, interactions and flows.
  • [x] Corner cases, error conditions, and easily imagined bugs.

nimishmedatwal avatar Apr 26 '24 19:04 nimishmedatwal

I am not sure if this test (and tests like this) in rendered_markdown.test.js needs to be changed since all the node-tests are passing.

run_test("code playground none", ({override, mock_template}) => {
    override(realm_playground, "get_playground_info_for_languages", (language) => {
        assert.equal(language, "javascript");
        return undefined;
    });

    override(copied_tooltip, "show_copied_confirmation", noop);

    const {prepends, $button_container, $view_code} = test_code_playground(mock_template, false);
    assert.deepEqual(prepends, [$button_container]);
    assert_clipboard_setup();

    assert.equal($view_code.attr("data-tippy-content"), undefined);
    assert.equal($view_code.attr("aria-label"), undefined);
});

nimishmedatwal avatar Apr 27 '24 16:04 nimishmedatwal

@amanagr are you up for reviewing this one? I haven't tested.

alya avatar Apr 30 '24 23:04 alya

LGTM

amanagr avatar May 06 '24 09:05 amanagr

Merged, thanks @nimishmedatwal!

timabbott avatar May 06 '24 22:05 timabbott

I guess @karlstolley FYI; this is a case where brittle JS made refactoring the HTML structure break stuff. I don't think there's an easy way to catch this sort of thing except experimentally, so I wouldn't spend undue additional time on refactoring. I don't think this type of bad pattern is common.

I guess we didn't fix the brittleness in this PR. @nimishmedatwal would you put up a follow-up that instead uses .closest() and .find() to find the right element without making assumptions about how many containing elements there are?

timabbott avatar May 06 '24 22:05 timabbott

I guess we didn't fix the brittleness in this PR. @nimishmedatwal would you put up a follow-up that instead uses .closest() and .find() to find the right element without making assumptions about how many containing elements there are?

Sure! I'll do that.

nimishmedatwal avatar May 07 '24 08:05 nimishmedatwal