Skip to content

Conversation

@PrimaMateria
Copy link
Contributor

Hi guys,
this is my first contribution to other github project and also first time I worked with react, so please be critical.
It is related to issue Rearrange storage #741.

Quick description:
From StorageItem extracted FolderList and FolderItem as separate components.
Wrapped them in the rect-sortable-hoc.
Implemented new data api method called reorderFolder.

I hope you will like it.
Cheers
Matus

@BoostnoteBot
Copy link
Collaborator

Please make sure to be pasted screenshots of all your changes.

@kazup01
Copy link
Member

kazup01 commented Sep 8, 2017

Firstly, could you fix the CI error?

@PrimaMateria
Copy link
Contributor Author

Here comes the screenshot:
screen shot 2017-09-08 at 14 29 44

Ok, I will later have look on the failed checks.

if (!_.isArray(rawStorages)) throw new Error('Target storage doesn\'t exist.')

targetStorage = _.find(rawStorages, {key: storageKey})
if (targetStorage == null) throw new Error('Target storage doesn\'t exist.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is findStorage in browser/lib and not in browser/main/lib/dataApi?
Besides my reorderFolder I also updated updateFolder.
There was one more possible candidate renameStorage, but I didn't want to touch it now in the scope of this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it just operates localStorage, does not operate files.

@@ -0,0 +1,48 @@
const test = require('ava')
Copy link
Contributor

Choose a reason for hiding this comment

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

Good 😄

@PrimaMateria
Copy link
Contributor Author

Tests are failing randomly. I put log into beforeEach and after for reorderFolder-test and moveNote-test and it seems like one test is clearing localStorage during the execution of other.

This is log from the failed run:

reorderFolder setting items to localStorage
moveNote setting items to localStorage
moveNote clearing localStorage

   19 passed
   1 failed


   1. dataApi › reorderFolder-test › Reorder a folder
   failed with "Cannot read property 'key' of undefined"
      Test.fn (reorderFolder-test.js:43:58)
    _combinedTickCallback (internal/process/next_tick.js:141:11)
    process._tickCallback (internal/process/next_tick.js:180:9)

And these are 2 logs from successful run:

reorderFolder setting items to localStorage
/tmp/test/reorder-folder/boostnote.json
moveNote setting items to localStorage
reorderFolder clearing localStorage
/tmp/test/update-folder/boostnote.json
/tmp/test/create-folder/boostnote.json
moveNote clearing localStorage
/tmp/test/create-folder/boostnote.json
reorderFolder setting items to localStorage
/tmp/test/update-folder/boostnote.json
/tmp/test/reorder-folder/boostnote.json
reorderFolder clearing localStorage
moveNote setting items to localStorage
moveNote clearing localStorage

You can see that order is always different.

@asmsuechan
Copy link
Contributor

Tests are failing randomly.

I know it. But The fixing cost >>>>>> Push the restart button.

@PrimaMateria
Copy link
Contributor Author

Hi guys, I am sorry, I don't know how the process works now.
Is there something left for me to do?

@asmsuechan
Copy link
Contributor

Is there something left for me to do?

No, there's nothing to say about your PR. I left cross-OS checks for my review but I'm busy in these days so I could not take a time to do it. I'll work on this PR again in a week.

@asmsuechan
Copy link
Contributor

Works fine! Thank you!

@asmsuechan asmsuechan merged commit f9a7c2d into BoostIO:master Oct 13, 2017
@kohei-takata kohei-takata mentioned this pull request Oct 28, 2017
@kazup01 kazup01 mentioned this pull request Oct 28, 2017
@kazup01
Copy link
Member

kazup01 commented Oct 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants