[Update] Support Page Sections & Sphinx Domains #19

dojutsu-user · 2019-06-29T14:23:11Z

Demo

Related PR in readthedocs.org -- readthedocs/readthedocs.org#5829

ericholscher · 2019-07-01T20:47:19Z

sphinx_search/_static/js/rtd_sphinx_search.js

+            outer_div.appendChild(section_content);
+            section_link.appendChild(outer_div);
+            content.appendChild(section_link);
+            content.appendChild(createDomNode("br", { class: "br-for-hits" }));


I really dislike this way of creating HTML. Is it possible for us to use a JS template language for this rendering, instead of all being in code? I feel that would be much, much cleaner.

I searched on the internet and found a very small and easy to use library -- squirrelly.js.
I have updated the PR to make use of it.
I think we can use it in other places also. So I'll be updating the code and use it where ever it proves to be effective.

I don't have a particularly strong opinion but sphinx docs already rely on underscore which has built-in templating. It may not be the right fit but it could avoid an external dependency.

@davidfischer
Thanks.
I have removed the external dependency and used the uderscore.js for it.

ericholscher · 2019-07-01T20:57:43Z

sphinx_search/_static/js/rtd_sphinx_search.js

+        }
+    }
+
+    if (domains !== undefined && domains !== null) {


Are we always showing sections before domains in this logic?

Yes.
It is because we are getting the results of sections and domains separately.
sections results are sorted and domains results are sorted -- but there is no sorting present which invloves both.

I believe ES should return the score for each object, and we're removing it in the web API. We could probably be passing along the score to the JS, so that we could use it more smartly for sorting.

Yes. Score is present in each result.
Where should the sorting be done? server side or client side?
I think it's better to do it in server side as we will be using the same structure and then we have to do sorting everywhere.

I think a server side sort is reasonable so that the search results on readthedocs.org and in-doc search mostly match.

I have implemented a server side sort. But I'm still concerned about its efficiency as I have not implemented any algorithm.

ericholscher

Looked over all the code (not the CSS) and it looks good. Just some simplification and refactors that should make it simpler.

ericholscher · 2019-07-08T22:00:06Z

docs/custom-design.rst

+                <!-- Results from other pages -->
+                <div class="search__result__single"><div>...</div></div>
+                <div class="search__result__single"><div>...</div></div>
+                <div class="search__result__single"><div>...</div></div>
            </div>
        </div>
    </div>


Maybe we should delete this content until we settle on the final HTML, then we can add it back? Alternatively, we could use literalinclude from Sphinx to include this directly from the HTML: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-literalinclude

I have removed it for now and we can target these things things in a separate PR to imrpove the docs and other things.
I don't think that we can use literalinclude here because content is being dynamically generated and removed from the DOM.

ericholscher · 2019-07-08T22:01:21Z

sphinx_search/_static/js/rtd_sphinx_search.js

+    }
+
+    let section_content =
+        sectionData._source.content.substring(0, 100) + " ...";


Do we want to substring this here? Perhaps we should do it in the API instead. I don't have a strong feeling here though -- why are we only using 100 chars?

I also don't have a particular opinion here.
But I think if we preprocess this on server side, it will reduce the size of the fetched data. So preprocessing this might decrease the time to get the results from server. What are your thoughts on this?

I used the substring with 100 because I think it is just sufficient data from a page which don't have any highlighted content.
We can increase or decrease this as per our choice. I have used this in other places also.

ericholscher · 2019-07-08T22:02:21Z

sphinx_search/_static/js/rtd_sphinx_search.js

+        sectionData.highlight["sections.content"].length >= 1
+    ) {
+        section_content =
+            "... " + sectionData.highlight["sections.content"][0] + " ...";


Is there a reason we're accessing only the first item of the list here? Should this be a constant about how many to include?

We were getting only one result per section. So there has to be only one element in the array.
But I have refactored the code now and it is much better. We can show as many results from a section as we want. Here's a demo:

I have used it with maximum 3 results from a section

Currently I have used the approach of getting the value at index 0 directly everywhere.
I will be refactoring them also. (search.html and main site search)

ericholscher · 2019-07-08T22:04:12Z

sphinx_search/_static/js/rtd_sphinx_search.js

+            domain_type_display =
+                domainData.highlight["domains.type_display"][0];
+        }
+    }


Can we write a function to abstract these 3 functions that do the same thing? get_highlight_list_data('domains.display_name)` or similar?

ericholscher · 2019-07-08T22:05:22Z

sphinx_search/_static/js/rtd_sphinx_search.js

+    }
+    if (
+        typeof domain_doc_display === "string" &&
+        domain_doc_display.length > 0


Can we add an _is_string function or similar that does this?

ericholscher · 2019-07-08T22:05:33Z

sphinx_search/_static/js/rtd_sphinx_search.js

+    if (domainData.highlight !== undefined && domainData.highlight !== null) {
+        if (
+            domainData.highlight["domains.name"] !== undefined &&
+            domainData.highlight["domains.name"].length >= 1


Can we add an _is_list function that does this?

ericholscher · 2019-07-08T22:05:55Z

sphinx_search/_static/js/rtd_sphinx_search.js

+    let domain_subheading = "";
+    if (
+        typeof domain_display_name === "string" &&
+        domain_display_name.length >= 2 // >= 2 because many display_names have values "-"


Can we fix indexing to not store this data?

I think we can.
I will look into this more.

@ericholscher
This is fixed now -- readthedocs/readthedocs.org@f988302

ericholscher · 2019-07-08T22:07:08Z

sphinx_search/_static/js/rtd_sphinx_search.js

+ *      </a>
+ *      <br class="br-for-hits">
+ *
+ *  </div>


Let's keep this in the docs, we don't need it here. It takes up a lot of space.

ericholscher · 2019-07-08T22:09:24Z

sphinx_search/_static/js/rtd_sphinx_search.js

+                page_link
+            );
+        } else {
+            if (type === "domains") {


should be else if ?

dojutsu-user · 2019-07-09T08:18:55Z

@ericholscher
I have updated the PR.

ericholscher

This is much better. I think we're really close to merging. 👍

ericholscher · 2019-07-09T19:31:22Z

sphinx_search/_static/js/rtd_sphinx_search.js

-        section_content =
-            "... " + sectionData.highlight["sections.content"][0] + " ...";
+    let section_content = [
+        sectionData._source.content.substring(0, 100) + " ..."


I'd like to see 100 be a constant defined at the top of the file, at least. We can keep that for now.

Updated the PR.

dojutsu-user · 2019-07-10T07:11:12Z

@ericholscher
I have also updated the PR to include minified files now.

dojutsu-user · 2019-07-11T05:37:47Z

@ericholscher
This is ready to be merged I think.

ericholscher

Agreed its ready to merge, just a few nitpicks, then feel free to merge.

ericholscher · 2019-07-11T15:44:41Z

MANIFEST.in

 include sphinx_search/_static/js/rtd_sphinx_search.min.js
-include sphinx_search/_static/css/rtd_sphinx_search.css


I do think we should include these as well as the minified ones. People generally like to be able to debug against non-minified source.

ericholscher · 2019-07-11T15:45:35Z

sphinx_search/extension.py

+    for file in CUSTOM_ASSETS_FILES:
+        if file.endswith('.min.js'):
+            app.add_js_file(file)
+        if file.endswith('.min.css'):


I wonder if we should have a setting that specifies minified or non-minified files here? We can ship like it is, but I almost always prefer full versions when doing dev.

ericholscher · 2019-07-11T15:46:04Z

tests/test_extension.py

@@ -36,9 +36,9 @@ def test_static_files_injected_in_html(selenium, app, status, warning):
    page_source = selenium.page_source

    assert (
-        page_source.count('rtd_sphinx_search.js') == 1
+        page_source.count('rtd_sphinx_search.min.js') == 1


these should be variables, not strings put thoughout the file.

dojutsu-user · 2019-07-12T08:50:16Z

@ericholscher
I have updated the PR.
I have added a configuration variable - RTD_SPHINX_SEARCH_FILE_TYPE whose possible values are - MINIFIED and UN_MINIFIED. User can set these two values in their conf.py and the corresponding css and js files will get added in the docs.
For now, the default value is set to UN_MINIFIED for debugging purpose.
We will change this value once we decide to ship it.
Also, the tests for the same are added.

ericholscher

Thanks for this addition. I think it makes sense with some convention changes 👍

ericholscher · 2019-07-12T16:01:56Z

sphinx_search/extension.py

+
+def setup(app):
+
+    app.add_config_value('RTD_SPHINX_SEARCH_FILE_TYPE', 'UN_MINIFIED', 'html')


I believe Sphinx uses lower-case config values by convention.

This also should be documented.

Can the documentation changes be done in another PR (the one with the API reference)?

ericholscher · 2019-07-12T16:02:44Z

sphinx_search/extension.py

-    os.path.join('css', 'rtd_sphinx_search.min.css'),
-]
+CUSTOM_ASSETS_FILES = {
+    'MINIFIED': [


these should also be lowercase.

ericholscher · 2019-07-12T16:03:01Z

sphinx_search/extension.py

-def setup(app):
+def inject_static_files(app):
+    """Inject correct CSS and JS files based on the value of ``RTD_SPHINX_SEARCH_FILE_TYPE``.""" 
+    file_type = app.config.RTD_SPHINX_SEARCH_FILE_TYPE


we should check the incoming value for being one of the two we expect, and give users a good error if not.

ericholscher

I believe this is ready to merge. The only thing left is to document the new setting.

dojutsu-user · 2019-07-14T07:00:54Z

@ericholscher
Updated the docs.

dojutsu-user added 2 commits June 29, 2019 19:38

Update the extension to support page sections and sphinx domains

e24214d

fix tests

8b56cee

dojutsu-user requested review from davidfischer and ericholscher June 29, 2019 14:27

dojutsu-user mentioned this pull request Jun 29, 2019

Add section linking for the search result readthedocs/readthedocs.org#5829

Merged

dojutsu-user added PR: work in progress Pull request is not ready for full review Needed: documentation Documentation is required labels Jun 29, 2019

dojutsu-user added 2 commits June 29, 2019 22:19

set count to zero

3cdedc7

set margin

a477e9c

dojutsu-user mentioned this pull request Jun 30, 2019

Link to search modal #20

Closed

update docs

eecb619

dojutsu-user removed Needed: documentation Documentation is required PR: work in progress Pull request is not ready for full review labels Jun 30, 2019

dojutsu-user added 3 commits June 30, 2019 20:39

change debounce time

03f1808

update debounce time

2173fc4

add min js file

66d8861

ericholscher reviewed Jul 1, 2019

View reviewed changes

dojutsu-user added 6 commits July 2, 2019 16:38

use templating langugage

31f142f

add missing bracket

abf809c

show sorted results

c115c52

add comments

24c84f5

Fix test

156a8b9

show more data for domain objects

d533cfc

dojutsu-user added the PR: work in progress Pull request is not ready for full review label Jul 3, 2019

dojutsu-user added 5 commits July 4, 2019 00:59

remove external templating language

73f3b4b

fix horizontal scroll bar

2d9763b

show subprojecs with font reduced

1e39d47

fix clashing css class

e4796a7

use more .template

6e869e2

don't use minified file

8dce55e

ericholscher reviewed Jul 8, 2019

View reviewed changes

dojutsu-user added 4 commits July 9, 2019 10:31

remove html structure comments and from the docs

c735b75

show max 3 results from page sections

047a438

improvements

d8c9d46

use helper functions

40fef13

ericholscher approved these changes Jul 9, 2019

View reviewed changes

dojutsu-user added 3 commits July 10, 2019 12:36

use 100 as constant

ab1dbbb

use minified files

2957f9a

update minified files

7c1dfcf

dojutsu-user added 6 commits July 10, 2019 20:47

add checkbox for searching in current section

dbddbd4

remove checkbox for now

bf7dfec

clean some code

ed8764e

fix tests

75a0cd9

Merge branch 'master' into update-extension-with-sections

46a383b

update minified files

2746b35

dojutsu-user mentioned this pull request Jul 11, 2019

Link To Search UI #25

Merged

ericholscher reviewed Jul 11, 2019

View reviewed changes

provide setting for choosing between minified and unminified files

b164cb2

ericholscher reviewed Jul 12, 2019

View reviewed changes

follow sphinx convention

ffa8bc3

ericholscher approved these changes Jul 12, 2019

View reviewed changes

update docs

a79e44d

ericholscher merged commit dff4877 into master Jul 15, 2019

dojutsu-user deleted the update-extension-with-sections branch July 15, 2019 18:43

		include sphinx_search/_static/js/rtd_sphinx_search.min.js
		include sphinx_search/_static/css/rtd_sphinx_search.css


		def setup(app):

		app.add_config_value('RTD_SPHINX_SEARCH_FILE_TYPE', 'UN_MINIFIED', 'html')

[Update] Support Page Sections & Sphinx Domains #19

[Update] Support Page Sections & Sphinx Domains #19

Uh oh!

Conversation

dojutsu-user commented Jun 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dojutsu-user Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericholscher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dojutsu-user commented Jul 9, 2019

Uh oh!

ericholscher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dojutsu-user commented Jul 10, 2019

Uh oh!

dojutsu-user commented Jul 11, 2019

Uh oh!

ericholscher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dojutsu-user commented Jun 29, 2019 •

edited

Loading

dojutsu-user Jul 2, 2019 •

edited

Loading

dojutsu-user Jul 12, 2019 •

edited

Loading