Feature/search form #106

thatbudakguy · 2018-08-07T15:08:34Z

no tests yet; working on that in Set up JS testing infrastructure #104
search components are written in a way as to make them semi-reusable for other search pages; will improve upon this and probably switch them to single-file component architecture as part of Rewrite vue components to use SFC structure #108

todos:

document components
document mutations/getters/actions
~~implement passing in component config from django template~~ tracked on Refactor search components to allow passing props & html in #109
~~add "clear facet" button~~ tracked on As a user, I want to clear all selections from a particular search facet so I can change parts of my search without losing the whole search. #110 ; moved to "nice to have"
remove translator/publisher
remove filters
add "clear all choices" button
make range facets work

codecov-io · 2018-08-07T21:12:11Z

Codecov Report

Merging #106 into develop will increase coverage by 0.28%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #106      +/-   ##
===========================================
+ Coverage    97.82%   98.11%   +0.28%     
===========================================
  Files          103      103              
  Lines         3685     4241     +556     
===========================================
+ Hits          3605     4161     +556     
  Misses          80       80

Impacted Files	Coverage Δ
winthrop/common/solr.py	`99.25% <ø> (-0.14%)`	⬇️
winthrop/books/forms.py	`98.79% <ø> (-0.02%)`	⬇️
winthrop/common/views.py	`86.2% <100%> (+3.59%)`	⬆️
winthrop/settings.py	`88% <100%> (+0.24%)`	⬆️
winthrop/books/views.py	`95.26% <100%> (+2.15%)`	⬆️
winthrop/common/templatetags/winthrop_tags.py	`100% <100%> (ø)`	⬆️
winthrop/books/tests/test_book_views.py	`100% <100%> (ø)`	⬆️
winthrop/common/tests/test_templatetags.py	`100% <100%> (ø)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 805bee4...adfd226. Read the comment docs.

rlskoeser

I switched to this branch, ran npm install, and was able to view and run the code. It's very satisfying to see the search results changing as I select names.

Sometimes clicking the checkbox seems not to register? Either that, or it requires that I am very precise and click only in the box. Can we make the labels clickable also?

As we discussed, when I click on book detail and then go back, I get a page with no styles in Chrome - I figured out this my fault, we need a Vary header. I'll add that shortly.

I think there might be a case where the load doesn't happen properly - when I go back from the page detail or when I reload a page with options set, the search filters aren't getting populated.

I added some comments the code, but the biggest thing I'd like to see is more comments describing the different components and how they relate.

rlskoeser · 2018-08-07T20:48:37Z

sitemedia/js/components/FilterChoice.js

@@ -0,0 +1,25 @@
+export default Vue.component ('FilterChoice', {


Should components have a doc string or comment? It seems like it would be helpful to orient in terms of context and use case or goals for a component.

I like this idea; makes sense to document them at that level. I'll add a docstring-like comment at the top of the files.

rlskoeser · 2018-08-07T20:49:10Z

sitemedia/js/components/FilterChoice.js

@@ -0,0 +1,25 @@
+export default Vue.component ('FilterChoice', {
+    template: `
+    <label :active="active" is="sui-button" role="checkbox" basic circular compact>


Missing class="..." here for basic circular compact?

so the is="sui-button" tells Vue that I'm using a different tag (label) when I actually want to render a button (sui-button), which would receive the basic circular compact boolean attributes at the end of the tag as props to sui-button, which ultimately renders:

<label class="ui button basic circular compact"></label>

this is coming from semantic-ui-vue. we can talk more about the library - it seems like it might not be well-maintained, but I'm not sure if there is an alternative to make the semantic UI behaviors play well with Vue.

rlskoeser · 2018-08-07T20:50:02Z

sitemedia/js/components/FilterChoice.js

+            :name="name"
+            v-model="active"
+            hidden
+        >


I prefer closed tags. Not sure if it actually matters here (I know it does for django templates if we want to use the assertContains with html)

rlskoeser · 2018-08-07T20:51:15Z

sitemedia/js/components/FilterChoice.js

+    `,
+    props: {
+        name: String,
+        label: String,


I guess label and value are different to allow for displaying facet counts? Maybe warrants a comment?

"label" is the user-friendly display name; "value" is the value attribute that gets used on the <input> element. I think if I document the components' props with a docstring like you mentioned this will be clearer.

rlskoeser · 2018-08-07T20:56:05Z

sitemedia/js/components/RangeFacet.js

+    template: `
+    <div class="range-facet">
+        <div class="inputs">
+            <sui-input :placeholder="minVal" />


It looks like sui-input is a semantic-ui vue element. I wonder if the approach & combination of tools needs to be written up briefly somewhere? Not sure where yet. Why sui-input here but not in FilterChoice?

FilterChoice actually isn't used currently since filters aren't implemented. I went back and forth between having a component to represent a single choice/checkbox (essentially a custom wrapper around <input>) vs just using <sui-input>...most of that struggle was prior/during/the cause of me implementing Vuex, because having the checkbox maintain its own state and simultaneously publish that state to the form wasn't working well.

The reason using semantic-ui vue is necessary at all is because the behaviors that semantic UI provides (like the js used for the interactivity on the site search bar and mobile menu) might not play well if used inside a Vue component.

rlskoeser · 2018-08-07T21:05:02Z

sitemedia/js/components/SearchFilter.js

@@ -0,0 +1,31 @@
+import FilterChoice from './FilterChoice'


Comments or docstrings would be really helpful to help understand the difference between SearchFilter and SearchFacet

rlskoeser · 2018-08-07T21:07:21Z

sitemedia/js/components/SearchSort.js

+        ]),
+        options() {
+            return [
+                {


Will this be pulled from / generated via the django form in future so we don't have to duplicate options?

Yep, that's my hope. Should be able to just pass it into the component using html.

rlskoeser · 2018-08-07T21:09:00Z

sitemedia/js/components/books/BooksSearch.js

+                    </search-facet>
+                </div>
+            </sui-segment>
+            <sui-segment inverted class="active-items">


Should only display when there are active items, right?

Do we have a design of the form with no options active? The spec for the Annotations search shows one without this section displayed...should it just go away completely when nothing is active?

rlskoeser · 2018-08-07T21:12:01Z

sitemedia/js/components/books/BooksSearch.js

+                ['annotator']
+            ],
+            activeTab: 0,
+            facetConfig: [


These would also be great to configure from the Django form somehow (I know we discussed that at some point)

Yeah, hoping to use the same approach here.

rlskoeser · 2018-08-07T21:14:16Z

sitemedia/js/components/books/BooksSearch.js

+        ]),
+    },
+    created() {
+        this.setEndpoint('facets/')


Can we make this configurable too? I'm imagining a few options being set in the template - not sure what makes the most sense. But we should set this via Django URL rather than hard-coding it here.

Yep, another thing we can pass in using html.

…op-django into feature/search-form

meg-codes

This is really slick. I had a few comments, one based on one getter that I'm a bit confused about but otherwise it's great to use.

I do note that if I toggle off the range selector tab I lose the year I picked, but I also gather that may be WIP.

meg-codes · 2018-08-08T18:53:52Z

sitemedia/js/components/SearchFacet.js

+        return {
+            filter: '',
+            alphaFilter: '',
+            alphabet: String.fromCharCode(...Array(91).keys()).slice(65)


Maybe this would be good to extrapolate away as a Vuex getter that takes an alphabetical facet's results and returns an alphabet with letters not present marked (or vice-versa)?

meg-codes · 2018-08-08T18:55:29Z

sitemedia/js/components/RangeFacet.js

+    },
+    methods: {
+        ...mapActions([
+            'setRangeFacet',


I am probably missing something here, but I see getters for setRangeFacetMin and setRangeFacetMax but not setRangeFacet.

Yeah, these are placeholders - I started out using a similar approach to the normal facets (one set function) but realized I needed two and haven't changed it yet. Coming soon...

rlskoeser · 2018-08-14T20:55:01Z

I have some comments, but I'm not sure any of them merit holding up this PR any further.

I expected to see a one or two line comment at the beginning of each component saying what it is / what it is used for. The method and property level documentation is much improved, but the higher level documentation would be good to add at some point.

The checkboxes are still difficult to click and clicking on the label doesn't toggle.

Publication range input doesn't have validation - not constrained to the min/max, doesn't require second date to be larger than the first. It also submits the form with a partially typed date if you are slow or thinking about what date you want - hopefully the same validation can address that.

An active publication date filter doesn't display on the list of selected filters, so if you're not on that tab it's not obvious the filter is in effect or easy to clear.

When I experimented with the publication dates, I also discovered a bug on the django side (my code) - 500 error when you try to load an empty result set, which is easy to do if you send nonsensical dates. I know what I need to fix.

rlskoeser · 2018-08-14T21:19:24Z

Couple more observations as I play with it more:

hitting enter in the keyword search submits the form (non-ajax submit / page reload)
active search term not displayed in the site search and site search is not visible by default; maybe this is an error with how we're setting up / binding the site search on the django/template side
Loading a page with no results makes the search filter look strange, since all the facets are empty. Not sure how best to handle (could try loading full facets without filters, but that might also be misleading)

thatbudakguy · 2018-08-15T15:36:00Z

moved high-level component documentation to #108
site-search related stuff moved to #116
validation moved to #115

range facets are handled and displayed more predictably adfd226
checkbox labels are clickable 8060dcc
enter no longer submits form a126e38
site search appears by default when loading the page b799f8d

thatbudakguy added 21 commits July 19, 2018 16:32

add vue.js & begin scaffolding search form

3b8cdef

books search form reactivity/serialization (#95)

a5b7ec5

add filter components and search/rolodex (#95)

b9a4843

first pass at active filter display (#95)

057aa9b

update dependencies, allow ES7

e1ffb47

manage form state using vuex (#95)

54e9521

rework vuex configuration (#95)

50087c8

active facets display (#95)

8e84910

querystring mutations and parser (#95)

3ae623d

basic range facet component (#95)

d198b6e

Merge branch 'develop' into feature/search-form

e329f1a

facet changes dynamically update choice counts (#95)

cec6722

working result fetching (#101)

605b734

total result count updates (#101)

0f286db

ajax result rendering (#101)

2c253eb

adjust sorting behavior for empty fields

80ed90a

setup sorting and keyword queries (#101)

d873bbd

adjust zero result test to match UI text

bc33f8c

Merge branch 'develop' into feature/search-form

ad3baa1

remove flat file example data

acc85b4

load initial results without js; add noscript tag

dabcf0d

thatbudakguy force-pushed the feature/search-form branch 2 times, most recently from c45c93c to 54eb4c3 Compare August 7, 2018 20:59

disable result count test

28f4a3d

thatbudakguy force-pushed the feature/search-form branch from 54eb4c3 to 28f4a3d Compare August 7, 2018 21:06

rlskoeser suggested changes Aug 7, 2018

View reviewed changes

rlskoeser added 2 commits August 7, 2018 17:38

Add Vary header to book list view to address ajax result caching issue

9d4f15d

Add book count to noscript fallback section and restore tests

9059917

thatbudakguy self-assigned this Aug 8, 2018

thatbudakguy added 2 commits August 8, 2018 10:25

close <input> tags

04e5600

Merge branch 'feature/search-form' of github.com:Princeton-CDH/winthr…

a654b60

…op-django into feature/search-form

meg-codes reviewed Aug 8, 2018

View reviewed changes

thatbudakguy added 3 commits August 8, 2018 16:03

add more explanatory comments

1ea2c44

update formState getter

9e06c33

remove publisher, translator, and filters

6219d8a

thatbudakguy force-pushed the feature/search-form branch from 5d1bd90 to 6219d8a Compare August 9, 2018 15:54

thatbudakguy added 10 commits August 9, 2018 16:07

refactor and document mutations & actions (#106)

3abbd9f

finish documentation, resolve routing issues

774a40f

pin dal to < 3.3

988c488

update semantic-ui-vue; refactor html to be more declarative (#109)

9625676

range facet handling

22902e6

pass in form label names from django (#109)

85e8fc9

function formatting

35de578

show disabled menu items

203b2e3

add json_dumps filter

c3f090e

add "clear all" button

804adf4

rlskoeser approved these changes Aug 14, 2018

View reviewed changes

thatbudakguy added 4 commits August 15, 2018 09:58

disable enter key submission for site search

a126e38

show site search by default on books search page

b799f8d

make checkbox labels clickable

8060dcc

improve range facet handling and display

adfd226

rlskoeser merged commit d9e0c72 into develop Aug 15, 2018

rlskoeser deleted the feature/search-form branch August 15, 2018 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/search form #106

Feature/search form #106

thatbudakguy commented Aug 7, 2018 •

edited

Loading

codecov-io commented Aug 7, 2018 •

edited

Loading

rlskoeser left a comment

rlskoeser Aug 7, 2018

thatbudakguy Aug 8, 2018

rlskoeser Aug 7, 2018

thatbudakguy Aug 8, 2018

rlskoeser Aug 7, 2018

rlskoeser Aug 7, 2018

thatbudakguy Aug 8, 2018

rlskoeser Aug 7, 2018

thatbudakguy Aug 8, 2018

rlskoeser Aug 7, 2018

rlskoeser Aug 7, 2018

thatbudakguy Aug 8, 2018

rlskoeser Aug 7, 2018

thatbudakguy Aug 8, 2018

rlskoeser Aug 7, 2018

thatbudakguy Aug 8, 2018

rlskoeser Aug 7, 2018

thatbudakguy Aug 8, 2018

meg-codes left a comment

meg-codes Aug 8, 2018

meg-codes Aug 8, 2018

thatbudakguy Aug 8, 2018

rlskoeser commented Aug 14, 2018

rlskoeser commented Aug 14, 2018

thatbudakguy commented Aug 15, 2018

		@@ -0,0 +1,25 @@
		export default Vue.component ('FilterChoice', {

Feature/search form #106

Feature/search form #106

Conversation

thatbudakguy commented Aug 7, 2018 • edited Loading

codecov-io commented Aug 7, 2018 • edited Loading

Codecov Report

rlskoeser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meg-codes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlskoeser commented Aug 14, 2018

rlskoeser commented Aug 14, 2018

thatbudakguy commented Aug 15, 2018

thatbudakguy commented Aug 7, 2018 •

edited

Loading

codecov-io commented Aug 7, 2018 •

edited

Loading