Separated the copy component and added functionality to copy code in the docs section #73
Conversation
|
@AnshumanDhiman is this PR same as #42? |
|
yes sir both the PR's are same, I was facing conflicting issues with the old PR and after force push it didn't meet up the expected changes. I had a conversation with Kartikey sir regarding this issue to open a new PR regarding this issue. |
Can you close your previous PR if it is no longer required? |
|
sure sir |
| @@ -491,7 +579,16 @@ const Docs = () => { | |||
| </a> | |||
| </h3> | |||
| <pre className='code language-shell'> | |||
| <code>npm install eos-icons-react</code> | |||
| <code className='copytext12 code-style'> | |||
There was a problem hiding this comment.
We have a coding convention for these type of scenarios where the class is merely used as an identifier for JS, but has no CSS styling at all. In these scenarios, we use the naming convention:
js-NAME
So, could you please add the prefix js- to all these copytextNN classes?
| const LinkData = [ | ||
| { | ||
| classname: '.copytext1', | ||
| link: 'npm install eos-icons --save' |
There was a problem hiding this comment.
I have to be honest. This is not a good practice. We are introducing a duplication of content and hence, a risk that this is not updated when something changes in the UI, thus the user would be copy-pasting old code.
We need to either use this object to populate the codesnippts, so, make this object the source. Or, my preferred solution, improve the copy function to grab the code that needs to be copied directly from its container.
|
@AnshumanDhiman always think about the scalability of your applications. All solutions need to be thought to not introduce human error when maintenance, new releases, new features come into play. Make sure your components are ready to be re-used. Make sure your data has 1 true source. Consider new actors that don't know the codebase and how they will scale or integrate with your applications. |
Thank you for your feedback, I will surely work on this and I will try from next time this thing won't happen again. |
|
@AnshumanDhiman can you merge master into your branch to refresh it? if you get too many conflicts, feel free to close this PR and start a new one too. Whatever is the easiest for you |
|
As I am having conflict issues here the code also have been updated far, I think I must close this PR and create a new PR on this. Thanks for helping😀 |
Fixed #39 Created a new PR, the old PR 42 was creating issues of conflicting, as discussed with @kbhutani0001 I have created a new file.