WebMemex / webmemex-extension

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

Added date time search feature enhancement . #66

Closed RajPratim21 closed 7 years ago

RajPratim21 commented 7 years ago

@Treora please reveiw my code regarding the commit, I have followd all the standard practises of redux of acrtions, reducers, store and tried to eliminate all the errors possible before submiting it. and also documented it and added required comments. issue #26

Treora commented 7 years ago

Great that you got this working solution. Having a working proof is only the first step though, next is to turn it into nice, readable, documented and maintainable code.

It looks like your code still uses the global variable workaround; did you perhaps forget to push the cleaned up approach you mentioned? There is also a huge amount of code debris, stuff that you probably changed while hacking but that should obviously be removed from the PR (even some parts of the date-picker docs appear to be copied into the readme?). Please have a look through the diff yourself, and try to make sure that every line in there is there for a reason. I would actually consider to start cleanly by checking out master, and one by one re-add the pieces that are part of the final solution.

We'll have to also find a nice way to use the css for the date picker without having to include it in our repo. Possibly the build process just has to copy it from node_modules/... into extension/overview.

RajPratim21 commented 7 years ago

Thank you @Treora for the appreciation, I will do the required to make the code "nice, readable, documented and maintainable" .

The Global variables are still required because I am using them to set the data at the reducer.js , once the date-pickers are clicked that event is caught by mapTodispatch and inside that onStartDateChange: date function
we set the Global variables with the current values Myclass.Global_startDate =date; and these Global variables are then accesed at reducer.js to change the state.startview. return Object.assign({},state, {endDate : MyClass.Global_endDate}) }

I will clear all the code debris and these Reame stuff. And regarding css stuff currently for this patcch I am not focussing on finding nicer way as that's out of scope of this patch. at least for now.

Treora commented 7 years ago

regarding css stuff currently for this patcch I am not focussing on finding nicer way as that's out of scope of this patch. at least for now.

Sure, I may look into it later.

Treora commented 7 years ago

Much better. It still looks like half the lines of the diff are there for no reason and should be removed, but the feature itself looks implemented sensibly now. For cleaning up the code, you could also start again from master and just add every line that is actually required for this code. Or if you feel you spent enough time on this already, I would totally understand; I can also do the final cleanup/rewrite myself, based on your approach.

RajPratim21 commented 7 years ago

@Treora Thank you for appreciating my work. It will be a big help if you can help with the cliean up as I am really interested in implementing the Natural language time search and wanted to focus on improvement of full text search and these stuffs as I plan to work on that. It would be very kind of you if you can do the clean up. I can later have a look on it and can learn how to cleanup as I am still naiva at it. Thank you @Treora .

RajPratim21 commented 7 years ago

@Treora I have added the required files to the original mster repo, and done the final code cleanup , I hope you can merge it now.

blackforestboi commented 7 years ago

Hey @RajPratim21

any reason why you changed 22k files? :)

RajPratim21 commented 7 years ago

@oliversauter , may be due to I added all the changes from a new reppo it caused the problem, I have deleted previous repo and clonned a new repo and added the required lines to it, it's working and test and code is clean and understandable , the PR is continued to PR #74

blackforestboi commented 7 years ago

Unfortunately now it's impossible to see what work you have done. :/ Please find a way to only see the changed files, maybe rebasing helps: https://git-scm.com/book/en/v2/Git-Branching-Rebasing

RajPratim21 commented 7 years ago

please refer to PR #74 for further evaluation. and can close this PR.