Open MuskaanMongia opened 9 years ago
awesome work @MuskaanMongia we are mostly using Solr and having this directly upstream in okfn/facetview would REALLY help! cc @darth-pr
Thank you professor. However, I feel there are still chances of improvement in UI and making it more robust. I will work and check in these changes soon. On May 3, 2015 11:56 PM, "Chris Mattmann" notifications@github.com wrote:
awesome work @MuskaanMongia https://github.com/MuskaanMongia we are mostly using Solr and having this directly upstream in okfn/facetview would REALLY help! cc @darth-pr https://github.com/darth-pr
— Reply to this email directly or view it on GitHub https://github.com/okfn/facetview/pull/47#issuecomment-98603232.
ping, is there any update here? thoughts about the PR?
This is great! Would love to see this merged here. One comment I would have is that it appears as though the facets in the index.html have been changed to work on the local example. This does not seem core to making it work with Solr. Essentially, I'm not seeing why the index.html file should change.
I'm not the maintainer, but a few comments on procedural aspects of the PR:
master
so it doesn't get unintentionally contaminated with later unrelated commits:+1: areed @tfmorris -> @MuskaanMongia can you submit an updated PR?
I have exams this week till Tuesday.. I wanted to submit a clean PR .. But could not get time .. Is it good if I do it on by 13 morning ? On May 9, 2015 12:54 PM, "Chris Mattmann" notifications@github.com wrote:
[image: :+1:] areed @tfmorris https://github.com/tfmorris -> @MuskaanMongia https://github.com/MuskaanMongia can you submit an updated PR?
— Reply to this email directly or view it on GitHub https://github.com/okfn/facetview/pull/47#issuecomment-100538769.
:+1: @MuskaanMongia
I have added a readme.txt for the config help. Please let me know if anything else is required to make it better
You're changing the permissions of the index.html file to include executable bits. Common problem if you're editing from a mounted NTFS or FAT32 partition in *nix and Mac. If you're on Windows your git client should be smart enough not to add the executable permissions to text files you're editing, otherwise all your files in all your repos will be executable...
You're moving the CSS files to the top of index.html, placing them BEFORE bootstrap is loaded. Why? This stops any bootstrap customisations contained within the facetview css from working.
Instead of modifying the index.html example, why not simply create a copy, index_solr.html? That way we don't lose the elasticsearch example.
Same for the primary JS file, why overwrite when you can just make a copy?
Why all the glyphicon-related and the bootstrap grid naming changes? Did you upgrade bootstrap?
This is probably best left as a separate fork rather than merged - conceptually it's not adding solr support, it's going in a different direction than the current master branch. I'd be happy to link to the fork from the project readme.
Finally (and I should update the project readme with this), there is more modular facetview at https://github.com/CottageLabs/facetview2 . It does not try to support solr, but such support may be easier added there than in this codebase - the code structure is better there.
These changes also allow facet view to be directly indexed with SOLR. by allowing cross origin source request in SOLR.