add/download screen #516

aks- · 2018-02-13T19:43:50Z

No description provided.

martinheidegger

Looks good @aks- , tested it out and everything works. The code is okay as well, except for the copy & paste job with the sparse data which is too much common code. Please extract that into a separate logic.

martinheidegger · 2018-02-20T01:53:47Z

app/actions/index.js

+  for (const con of dat.network.connections) {
+    con.removeAllListeners()
+  }
+  dat.stats.removeAllListeners()


While it seems necessary that we do this, I kinda thought that dat.close() should do that 🤔

martinheidegger · 2018-02-20T01:56:17Z

app/actions/index.js

+
+    dispatch(updateState(dat))
+
+    if (!paused) {


Paused is always false!

martinheidegger · 2018-02-20T01:57:54Z

app/actions/index.js

+      dispatch({ type: 'DAT_METADATA', key, metadata })
+    })
+
+    const walk = () => {


This seems (unnecessarily) copy & pasted from addDat. Please extract into common code.

juliangruber · 2018-02-20T11:13:25Z

app/actions/index.js

+      dispatch({ type: 'DAT_METADATA', key, metadata })
+    })
+
+    const walk = () => {


juliangruber · 2018-02-20T11:13:58Z

app/actions/index.js

+
+    dispatch(updateState(dat))
+
+    if (!paused) {


juliangruber · 2018-02-20T11:14:16Z

app/components/download.js

+          <div className='flex ml2'>
+            <GreenButton
+              onClick={() => {
+                addDat({ key: dat.key, path: dat.path, paused: false })


we should default paused to false

juliangruber · 2018-02-22T13:07:10Z

this still needs some work, I understand it's not an easy case

aks- · 2018-02-22T14:50:03Z

@juliangruber oh, you'd done it, i was just getting to it 🙌

aks- · 2018-02-22T15:02:44Z

app/actions/index.js

@@ -134,8 +128,6 @@ export const downloadSparseDat = ({ key }) => dispatch => {
    dispatch(updateState(dat))
    joinNetwork(dat)(dispatch)
    updateConnections(dat)(dispatch)
-
-    dats[key] = { dat, path, opts }


@juliangruber how do you remove this dat and close it when you click cancel download if you don't keep reference to it somewhere?

you're right, I misread the code

juliangruber · 2018-02-26T07:20:23Z

ok, sorry about that, unbroke it now

juliangruber · 2018-02-26T07:20:50Z

app/actions/index.js

@@ -33,6 +33,106 @@ export const createDat = () => dispatch => {
  addDat({ path })(dispatch)
 }

+export const showDownloadScreen = key => ({


what do you think about merging this action with downloadSparseDat?

@juliangruber you mean merge the logic of showDownloadScreen into downloadSparseDat? I kept it separate because they do two different things? one opens up the screen and second one initiates the download of dat info.

aks- · 2018-04-17T09:39:00Z

@martinheidegger i checked it out... you mentioned there issues mentioned below

The preview will download data into a folder - so its not actually a preview.

The Download folder is static (it needs to be dynamic for another PR

Dats can never be downloaded twice because "dat" is our unique, why would we download dats more than once? Versions!

it does not download data into the folder, it only downloads metadata to show on download screen.
i'm not quite sure if i fully understand this one.
i agree, but we don't have a way to tell users as of now that they are trying to download an existing dat even in our master. this pr behaves similar to what is there on master. (cc: @Kriesse)

okdistribute · 2018-04-17T13:47:39Z

@aks- on point 3, I guess you could iterate through existing dats and see if one still exists with that id.

aks- · 2018-04-17T14:10:29Z

@Karissa right, but what action do we take once we find out that one we are trying to download already exist. don't we have to notify user of that?

aks- · 2018-04-19T05:40:57Z

@martinheidegger can you please take a look at this one again? i think we can merge it

martinheidegger · 2018-04-19T06:12:59Z

@aks- My review is contained in #529 and #530 respectively.

martinheidegger · 2018-04-21T03:10:23Z

Now this PR contains problems if the same dat is downloaded twice. I think that is the last issue before we should merge it to react?!

aks- · 2018-04-21T17:20:33Z

@martinheidegger that sounds right, tomorrow i'm travelling, i can work on it day after tomorrow 👍

That review is based on a old version.

… necessary.

martinheidegger · 2018-06-29T03:16:58Z

So: I gave this another shot. @soyuka It would be nice if you could review this once more. I would love to do a rebase after merging #541 :)

…load # Conflicts: # app/actions/index.js # app/components/table-row.js # app/reducers/index.js

martinheidegger · 2018-06-29T13:05:05Z

@soyuka Thank you for reviewing #541, I merged it in this branch. Can you give this branch another look?

Its outdated

Merge/react add download

martinheidegger · 2018-10-09T05:05:45Z

@AtuyL thank you for the fixes, would you mind to review this PR officially ;)

soyuka

Code looks good to me.

tests/utils/waitForMatch.js

app/components/button.js

package.json

aks- requested review from juliangruber and martinheidegger February 13, 2018 19:44

martinheidegger previously requested changes Feb 20, 2018

View reviewed changes

juliangruber previously requested changes Feb 20, 2018

View reviewed changes

aks- commented Feb 22, 2018

View reviewed changes

juliangruber reviewed Feb 26, 2018

View reviewed changes

martinheidegger force-pushed the add/download branch 2 times, most recently from 3a44f4a to af328a1 Compare April 16, 2018 05:52

aks- requested a review from martinheidegger April 19, 2018 05:41

martinheidegger force-pushed the add/download branch 4 times, most recently from 2f81ef0 to 38a5b8e Compare June 13, 2018 13:39

martinheidegger mentioned this pull request Jun 14, 2018

Add local gateway to allow browsers access to the network. #518

Open

aks- and others added 4 commits June 14, 2018 15:48

add/download screen

5b3e113

refactor always true conditional

d680d86

paused defaults to falsy

b636749

refactor walk fn

1ca6b58

Added single-unit script that allows to run a single-unit testcase if…

472cf38

… necessary.

martinheidegger force-pushed the add/download branch from ecedcbd to 472cf38 Compare June 29, 2018 03:14

martinheidegger changed the title ~~[wip] add/download screen~~ add/download screen Jun 29, 2018

Lint fix

7205ba7

martinheidegger added 3 commits June 29, 2018 20:39

Merge commit '472698a1e711b9e9e85b6f3b316d5861619ccd85' into add/down…

536b53b

…load # Conflicts: # app/actions/index.js # app/components/table-row.js # app/reducers/index.js

Lint fix

d17dfd5

Fixed merge mistake

67dd5aa

martinheidegger assigned AtuyL Oct 3, 2018

AtuyL and others added 6 commits October 3, 2018 16:57

removed: package-lock.json

fc522b6

back to home-screen on the action of DIALOGS_DELETE_CLOSE.

77d55f2

Fixed: merge mistake.

4830c33

fixed: missing package.

668af34

fixed: missing deps.

01d46d5

Merge pull request #554 from dat-land/merge/react-add_download

d9f1369

Merge/react add download

martinheidegger requested a review from AtuyL October 9, 2018 05:05

soyuka previously approved these changes Oct 9, 2018

View reviewed changes

tests/utils/waitForMatch.js Show resolved Hide resolved

tests/utils/waitForMatch.js Show resolved Hide resolved

app/components/button.js Show resolved Hide resolved

AtuyL reviewed Oct 11, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

apply: #516 (comment)

47abdb2

AtuyL dismissed soyuka’s stale review via 47abdb2 October 12, 2018 06:36

AtuyL mentioned this pull request Oct 12, 2018

Add/download #557

Closed

Merge branch 'react' into add/download

b6a8953

soyuka approved these changes Oct 12, 2018

View reviewed changes

AtuyL merged commit 7f40bc6 into react Oct 12, 2018

dkastl deleted the add/download branch October 13, 2018 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add/download screen #516

add/download screen #516

aks- commented Feb 13, 2018

martinheidegger left a comment

martinheidegger Feb 20, 2018

martinheidegger Feb 20, 2018

juliangruber Feb 20, 2018

martinheidegger Feb 20, 2018

juliangruber Feb 20, 2018

juliangruber Feb 20, 2018

juliangruber Feb 20, 2018

juliangruber Feb 20, 2018

juliangruber commented Feb 22, 2018

aks- commented Feb 22, 2018

aks- Feb 22, 2018

juliangruber Feb 26, 2018

juliangruber Feb 26, 2018

juliangruber commented Feb 26, 2018

juliangruber Feb 26, 2018

aks- Feb 26, 2018

aks- commented Apr 17, 2018

okdistribute commented Apr 17, 2018

aks- commented Apr 17, 2018

aks- commented Apr 19, 2018

martinheidegger commented Apr 19, 2018

martinheidegger commented Apr 21, 2018

aks- commented Apr 21, 2018

martinheidegger commented Jun 29, 2018

martinheidegger commented Jun 29, 2018

martinheidegger commented Oct 9, 2018

soyuka left a comment

add/download screen #516

add/download screen #516

Conversation

aks- commented Feb 13, 2018

martinheidegger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliangruber commented Feb 22, 2018

aks- commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliangruber commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aks- commented Apr 17, 2018

okdistribute commented Apr 17, 2018

aks- commented Apr 17, 2018

aks- commented Apr 19, 2018

martinheidegger commented Apr 19, 2018

martinheidegger commented Apr 21, 2018

aks- commented Apr 21, 2018

martinheidegger commented Jun 29, 2018

martinheidegger commented Jun 29, 2018

martinheidegger commented Oct 9, 2018

soyuka left a comment

Choose a reason for hiding this comment