Skip to content

Comments

Solution for issue: 4077#4412

Closed
Vivek-Kamboj wants to merge 2 commits intoelastic:masterfrom
Vivek-Kamboj:4077
Closed

Solution for issue: 4077#4412
Vivek-Kamboj wants to merge 2 commits intoelastic:masterfrom
Vivek-Kamboj:4077

Conversation

@Vivek-Kamboj
Copy link
Contributor

@Vivek-Kamboj Vivek-Kamboj commented Jan 5, 2021

Summary

Hi,

I have recreated this issue #4077 and I discovered that it is not present in HTML as shown below:

Before the expand button clicked:
Before

After the expand button clicked:
After

In my opinion, the error causing code is:
problemCode

Here, in my opinion, "textToCopy" is undefined when it renders which is creating all these issues. And when we expand it, the component re-renders and the copy button got displayed.

So I am thinking of removing "textToCopy" from the if statement.

My Changes:
Mychanges

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Hey @Vivek-Kamboj, thanks for the PR!

Although these changes do result in the copy button being shown, it does not result in the required functionality. Mainly, clicking the copy button will not copy the actual contents of the code block.
What we need is to find the correct lifecycle event coordination that will allow for textToCopy to equal the code block contents when the code block is visible.
My initial guess is that there needs to be an extra bit of state, or that lifecycle events related to the portal need refactored.

@Vivek-Kamboj
Copy link
Contributor Author

Hi @thompsongl,

Actually, I have made some changes in my local but I don't know how should I test this library for this issue on local or on the sandbox.
Can you guide me on how should I proceed with testing?
You can connect with me between 11 am(IST) to 11 pm(IST).

@thompsongl
Copy link
Contributor

It should be pretty straightforward to run this locally.
You'll need to clone the repo, and follow the Running Locally directions.
Then you can modify one of the documentation examples (e.g., accordion) to replicate the scenario shown in the codesandbox.io link.

@Vivek-Kamboj
Copy link
Contributor Author

As written in the comment that innerText could correctly be null but textContent could correctly be non-null due to differing reliance on browser layout calculations.

solution

So I propose using textContent instead of innerText.

After applying this:
After

Copy icon is visible and is working!

@thompsongl Is it a correct solution?

@thompsongl
Copy link
Contributor

So I propose using textContent instead of innerText.

Unfortunately this would change the existing behavior of EuiInnerText and all components that rely on it. We're looking for a solution scoped to EuiCodeBlock as it is the only component adversely impacted when used with EuiAccordion.
Relatedly, #4400 may resolve the issue by removing the need for a portal. I think that branch is worth exploring, even in its work-in-progress state.

@cchaos
Copy link
Contributor

cchaos commented Feb 18, 2021

Hi @Vivek-Kamboj , are you still working on this or would you like us to finish it up?

@Vivek-Kamboj
Copy link
Contributor Author

@cchaos, I don't understand the solution for this issue. So, I am unable to fix this issue.
You can finish it.
Sorry for the late response.

@cchaos
Copy link
Contributor

cchaos commented Feb 19, 2021

Thanks for the attempt @Vivek-Kamboj. Feel free to grab any other good-first-issues.

I'm actually just going to close the PR since the solution isn't clear.

@cchaos cchaos closed this Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants