Skip to content
This repository has been archived by the owner on Apr 8, 2019. It is now read-only.

Only copy URL to clipboard when !open #36

Merged
merged 1 commit into from
Mar 6, 2018
Merged

Only copy URL to clipboard when !open #36

merged 1 commit into from
Mar 6, 2018

Conversation

billyjanitsch
Copy link
Contributor

@billyjanitsch billyjanitsch commented Mar 2, 2018

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

This patch is a partial solution for #33: it disables the clipboard feature if the user has configured the open option (which clearly makes the clipboard feature redundant).

It doesn't introduce a way to configure the clipboard feature or otherwise change the default, pending more discussion in #33.

Additional Info

@codecov
Copy link

codecov bot commented Mar 2, 2018

Codecov Report

Merging #36 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #36   +/-   ##
=======================================
  Coverage   91.89%   91.89%           
=======================================
  Files           7        7           
  Lines         259      259           
=======================================
  Hits          238      238           
  Misses         21       21
Impacted Files Coverage Δ
lib/server.js 87.69% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c492f55...3c4bea1. Read the comment docs.

@billyjanitsch
Copy link
Contributor Author

CI wants me to add a test. Fair enough :)

@billyjanitsch
Copy link
Contributor Author

@shellscape how would you prefer me to test a config option like this? By adding a new config fixture to test/fixtures/? By modifying the basic one in an individual test?

@shellscape
Copy link
Contributor

Should be able to sneak that into this test, just checking that the clipboard is empty: https://github.com/webpack-contrib/webpack-serve/blob/master/test/tests/options.js#L135

And you can modify the then like so to double check the options was mutated properly:

serve({ config }).then(({ close, options }) => {

@shellscape
Copy link
Contributor

@billyjanitsch I just merged a PR that makes the clipboard stuff an option, so you'll probably need to rebase for that. We should also add a note to the README within the open option block that states that the clipboard copying is disabled when using the open option.

@billyjanitsch
Copy link
Contributor Author

Awesome! Sorry for the delay on this PR, I'll finish it up today.

@billyjanitsch
Copy link
Contributor Author

@shellscape let me know how that looks -- happy to make further changes

README.md Outdated
@@ -284,6 +284,8 @@ that matches:
}
```

This option disables the `clipboard` option.
Copy link
Contributor

@shellscape shellscape Mar 6, 2018

Choose a reason for hiding this comment

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

I'd change this to:

_Note: Using the `open` option will disable the `clipboard` option._

For emphasis


serve({ config }).then(({ close }) => {
hook = (...args) => {
assert.equal(clip.readSync(), 'foo');
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good 👍

can you add a comment above this line to note that it tests that the clipboard option is disabled when open is used?

@billyjanitsch
Copy link
Contributor Author

@shellscape done, let me know if there's anything else!

@shellscape shellscape merged commit 346530e into webpack-contrib:master Mar 6, 2018
@billyjanitsch billyjanitsch deleted the clipboard branch March 6, 2018 20:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants