WebMemex / webmemex-extension

📇 Your digital memory extension, as a browser extension
https://webmemex.org
Other
208 stars 45 forks source link

UI for history import (fix #17) #65

Closed alxmiron closed 7 years ago

alxmiron commented 7 years ago

Functionality

Since this PR is about UI only, I didn't change/implement any loading or parsing logic as it can be serious work in separate issue. Here are just implemented components and containers, that get statistics (showed within table) from props and dispatches actions.

When "Start import" button is clicked, startLoadingHistory action is dispatched. While loading, user can click "Pause", that dispatches stopLoadingHistory action with status paused. After that user can resume import by "Resume" button, that dispatches resumeLoadingHistory action.

Bookmarks loading and action handlers for it were not implemented as bookmarks import functionality not finished

Current design

1

Please, review all comments about the code


I have some global suggestion to the whole project. I don't know where to write them, I'll leave them here

Suggesstions

Will be glad to discuss all written above

17

blackforestboi commented 7 years ago

Hey @AlexMarvelo

Thanks for getting this running so quickly! I wonder if you have seen the mockups I made for the work-flow as yours differs so much from it.

The workflow how it is designed in the mockups:

  1. User gets information about download duration & Size
  2. User decides if he wants to import bookmarks and/or history items and confirms selection
  3. Items are requested from chrome history/bookmarks API and stored in the DB (Stage 1 of download, can be expressed in your progress bar, good idea)
  4. For each page the HTML is downloaded, parsed and the content stored to the pageObject with that URL. (stage 2 of download)
  5. Items where download failed make the counter increase (as seen in the mockups) and flagged accordingly in db. List of failed items is displayed in while downloading runs and afterwards.

In any case, if you have improvements to this work-flow, it is not set in stone.

Thanks a lot. :)

alxmiron commented 7 years ago

@oliversauter Ok, just haven't seen that mockup. Will complete it in couple days according to your workflow

obsidianart commented 7 years ago

@AlexMarvelo Can you move your suggestion in a more appropriate place? Maybe @oliversauter can help with that. Possibly split them if they can be implemented separately.

blackforestboi commented 7 years ago

@obsidianart I'll think of something, good idea.

Alex already mentioned though that he doesn't know where to put it, so I'll take the blame because we have not yet a place for such suggestions :)

Maybe a new issue with the tag code-style would be suitable? @Treora

Treora commented 7 years ago

Hi @AlexMarvelo, thanks for the effort and suggestions!

Regarding your questions and suggestions unrelated to this PR, it may be easiest to have a chat (#webmemex on Freenode IRC, or in WorldBrain's Slack if that's easier).

Regarding restructuring the options page

You may want to sync with @aquibm, who created the options page, and is also adding a redux store to it. The organisation of components/containers could be changed, making stateless functions of React classes has my preference too, but perhaps this is worth doing as a separate PR.

Note that the options page and overview are currently two completely separate React/Redux apps; this may answer some of your questions about why Redux code and explanation is within the overview folder rather than the root of the source tree. The organisation could still be improved though, as it is indeed rather contrary to people's expectation, and when options page has many parts in common with the overview the situation changes.

Regarding making importHistory cancellable

I see you found a solution to make it possible for the user to cancel the history import process, by making the import function a Redux thunk that reads from the state whether to continue. I admire the creativity, but I have the feeling that the feature is maybe not worth the complexity that is created. Importing the browser history (not talking about fetching the pages here) should in my experience take at most a few seconds, and for an MVP I deem it acceptable to not have a cancellation option for that. What do you think?

If we do want to cancel the process (now or later), it may be worth looking into good programming patterns to achieve this, preferably without making the import function too coupled to the rest of the application (to keep it reusable and maintainable). At the least, the checks could be factored out of the import function by just passing it a second argument shouldContinue: () => (getState().import=='pending') (or similar). Looking a bit further, redux-saga has a cancellation pattern built in, but I'm not too eager to adopt sagas just for that. I am not too enthusiastic about creating a solution with redux-observable either, as it does not seem to make code more readable. Doing some simpler generator magic might work; I am not an expert at this.

By the way, in any case, an improvement would be to convert the promises to use async/await syntax (#7).

As for the performance of the import process, I wonder if it helps if the browser api is called in sequence rather than in parallel when searching the visits to each historyItem.

alxmiron commented 7 years ago

Hi @Treora and @oliversauter

PR description was changed. Check it, please

Regarding making importHistory cancellable I agree that promise cancellation need a better solution. I can suggest bluebird as it's easy to integrate in our project and has obvious behavior, unlike redux-observable. Anyway, such work should be done in separate issue.

I want to underline, that this PR is about UI, all data is fetched from props only. Solution for hard drive size and time estimating, loaded pages amount etc require weighty improvement in import chain.

Treora commented 7 years ago

@AlexMarvelo: Hi, this PR was going in the right direction, did you by any chance work on this further after our last chat? Or planning to? I suppose some things may be easier to tackle after (or in harmony with) the blacklist feature (#20, #84) , since it also adds a redux store to the options page.

poltak commented 7 years ago

I've continued on from this in the feature/import-ui branch in the WorldBrain fork. Done a bit of refactoring so far, but will be able to use the presentational and state code from @AlexMarvelo's commit

Treora commented 7 years ago

Seems like this PR can be closed then.