Conversation
|
Travis automatic deployment: |
|
Travis automatic deployment: |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
@mikheevm ready for review |
src/utils/CopyToClipboard/index.tsx
Outdated
| className?: string; | ||
| }; | ||
|
|
||
| const CopyToClipboard = ({ |
There was a problem hiding this comment.
CopyBtn? feels weird to have copyToClipboard and CopyToClipboard in the same file
There was a problem hiding this comment.
I think CopyToClipboard is much explicit than just CopyBtn. I renamed the function instead.
There was a problem hiding this comment.
Explicit about what? What is CopyToClipboard component? A button? A text which would be copied if you click on it?
| @@ -0,0 +1,51 @@ | |||
| import React, { useState } from 'react'; | |||
There was a problem hiding this comment.
Why is this placed in utils? Shouldn't it be in inputs like all other buttons are?
There was a problem hiding this comment.
I see it more like and utility than input, but I don't have strong opinions about it
There was a problem hiding this comment.
Although all the components inside the input category should be used inside a form. This is not the case.
There was a problem hiding this comment.
So we shouldn’t use the button from inputs for triggering scripts? Can we point this in a documentation?
There was a problem hiding this comment.
I think the structure of the components should be refactored, could use evergreen as an example: https://evergreen.segment.com/components
There was a problem hiding this comment.
So we shouldn’t use the button from inputs for triggering scripts? Can we point this in a documentation?
I think we should, what I want to say is that input category is more related (not exclusive) to inputs form. As I user of the lib, if I want to search for a copy button, I would search first in utilities before inputs. But again, just my opinion.
There was a problem hiding this comment.
I think the structure of the components should be refactored, could use evergreen as an example: https://evergreen.segment.com/components
We discussed this before and we decided to follow the material-ui structure. but sure, we can change that, please create an issue and we can reorganize the categories.
|
Travis automatic deployment: |
|
Travis automatic deployment: |
2 participants
Closes #38.