removing element after selection #733
Conversation
|
For now, I just skip 2 tests, in my opinion, we can change the class to be a little more specific with the methods so we can isolate the dom node creation from the fake select method since we have plans to update this, my next PR will make this class more composable =) I would like to know you guys opinions |
vitormalencar
requested review from
helderberto,
ktfth,
obetomuniz and
zenorocha
| @@ -75,6 +73,8 @@ class ClipboardAction { | |||
|
|
|||
| this.selectedText = select(this.fakeElem); | |||
| this.copyText(); | |||
|
|
|||
| this.removeFake(); | |||
There was a problem hiding this comment.
Great! Simple solution. 💯
|
Just for reference @vitormalencar, why are you skipping the two mentioned tests instead of getting them in compliance with the bugfix? Could you pls elaborate on why each should be ignored for now on this PR? |
|
@obetomuniz the problem is that the test expects the node to stay in the dom, now that we are removing it we cant do assertions on the 'fakeElement, my idea is to separate this functionality a bit by having a new method just for the 'fakeElement' creation so that we can test properly, the idea here was to bring the discussion =) |
|
Got it. What about instead of only fix the bug, we start these improvements right here to avoid losing the test coverage? I mean, since it looks like a specific case, we can properly improve it as you mentioned instead of only this hotfix. What do you guys think? Let me know if you have concerns to do it here in terms of effort, time, etc. |
|
for me is totally ok =). the main point was to bring the discussion and I totally agree, I will create another method to keep our test coverage |
|
Awesome! Go for it. I'll keep tracking. Thanks, dude! |
…re-732-removing-dom-el * 'master' of github.com:zenorocha/clipboard.js: refactor(workflows): remove unused lint file feat(workflows): add lint code job chore(eslint): add comments and new rules chore(deps): remove sort-package-json refactor: remove eslint ignore rules comments ci(lint): create a ci workflow chore(clipboard): remove linter bugs chore(deps): add dependencies and new scripts chore(test): remove linter bugs chore(linter): add linter configuration feat(eslint): add linter configuration chore(deps): add linter updating naming adding deploy action
|
Nice !, team thanks for the review, let's go! |
Closes Issue #732
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
devbranch for v2.x (or to a previous version branch), not themasterbranchfix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information: