Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds faceted search #261

Merged
merged 28 commits into from
Jan 23, 2021
Merged

Adds faceted search #261

merged 28 commits into from
Jan 23, 2021

Conversation

garbas
Copy link
Member

@garbas garbas commented Jan 7, 2021

This PR add faceted (filtered) search for packages.

At the same time it changes the layout of the results, from table to ordered list. While the design is not final, the structure of the result items should give hint how this is going to be styled with https://github.com/nixos/nixos-common-styles.

A lot of inspiration was taken from github's search -> https://github.com/search

Fixes #163 #261 #95 #201

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2021

Copy link
Member

@turboMaCk turboMaCk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garbas let me know if I can (once I can) help with anything.

src/index.less Outdated
padding: 0;
}

ul.nav-stacked > li > a {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using class as oppose to structure might scale better but it can be improved later,

@garbas
Copy link
Member Author

garbas commented Jan 8, 2021

@garbas let me know if I can (once I can) help with anything.

I need to fix the query a bit but then I would really welcome the help how to make faceted search easier to use. I need to do a bit of a research in this area.

@samueldr
Copy link
Member

samueldr commented Jan 8, 2021

Can we have a description to the PR explaining what is the intent behind the change? So we can validate it works as intended? :)

@github-actions
Copy link
Contributor

@garbas
Copy link
Member Author

garbas commented Jan 11, 2021

Can we have a description to the PR explaining what is the intent behind the change? So we can validate it works as intended? :)

Oh sorry, I thought I opened a draft. Anyway add the description now.

@garbas garbas changed the title Faceted search Adds faceted search Jan 11, 2021
@github-actions
Copy link
Contributor

@garbas
Copy link
Member Author

garbas commented Jan 11, 2021

@samueldr @turboMaCk This is now ready for review.

@samueldr
Copy link
Member

samueldr commented Jan 12, 2021

Changing the Sort criteria affects the URL, but does not change it in the combo box, and does not seem to affect the results. Though, refreshing with the URL affected seems to work. Looks like something isn't causing the refresh of the results to be sent.

  • (check when fixed)

@samueldr
Copy link
Member

samueldr commented Jan 12, 2021

The clickable area for a result is... odd...

image

In green is the clickable area.

Suggestions:

  • On hover add a background to the whole li.option to make it more obvious it is clickable; some light (but not too light) gray I guess, or maybe light bootstrap blue?
  • Change from margin-bottom: 2em to padding-top: 2em. (You may need to adjust the listing top and bottom margin.)
  • (check when fixed)

@samueldr
Copy link
Member

samueldr commented Jan 12, 2021

Platforms listing in a result isn't filtered to useful platforms anymore.

This makes for useless verbosity:

image

The current search filters platforms for those that are... pragmatically useful? At least those with binary cache coverage.

  • (check when fixed)

Also note that the platforms listed in the facets (left bar) might also somewhat face the same issue. I don't really know how useful it is to show all of those platforms Nixpkgs doesn't actually support.

@samueldr
Copy link
Member

samueldr commented Jan 12, 2021

Package metadata is hard to understand. I guess that it is attribute name, version, for the first elements, then the license, homepage and source links are oddly not link-like.

Two main issues, first is there is no labeling on what the thing is, then there is no way to know that a thing is a link without hovering.

Sadly, I don't really know how to better solve it for being useful at a glance. The table-like layout used previously really is the pragmatic approach, it is hard to misunderstand.

image

  • (check when fixed)

@samueldr
Copy link
Member

samueldr commented Jan 12, 2021

I assume all narrow-viewport issues are to be looked at with the nixos-styles redesign. It's hard to fix all of this in a narrow layout.

Though, if not prohibitively expensive, it would be nice to have the side bar (with the facets) somehow be a togglable thing when narrow.

(No actual need to fix before merging, but something to keep in mind.)

@samueldr
Copy link
Member

samueldr commented Jan 12, 2021

Having an item opened at the bottom of the page, then changing facets scrolls to the opened item.

I guess that comes from the handling of navigation to a deep URL. I don't know how easy this is going to be to fix. To reproduce:

@samueldr
Copy link
Member

samueldr commented Jan 12, 2021

It is basically impossible to select information in an opened item. This is because the selection click, on release, hides the item. Not useful to select those installation instructions!

I guess the solution is somewhat linked to the problematic activation area issue (where changing the background colour would help). It might be necessary to do like the current implementation, and only close when the always present "header" is clicked. To help users, a "useless" "× close" button could be added too.

  • (check when fixed)

@@ -148,259 +190,357 @@ update navKey msg model =
view : Model -> Html Msg
view model =
Search.view { toRoute = Route.Packages, categoryName = "packages" }
"Search NixOS packages"
[ text "Search more than "
, strong [] [ text "80 000 packages" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any strategies to help ensure this number doesn't drift too far?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would be done as a counter that counts from 0 to a number which is fetched from the backend meanwhile. But for now this was the easier hack.

@@ -111,126 +111,64 @@ update navKey msg model =
view : Model -> Html Msg
view model =
Search.view { toRoute = Route.Options, categoryName = "options" }
"Search NixOS options"
[ text "Search more than "
, strong [] [ text "10 000 options" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any strategies to help ensure this number doesn't drift too far?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are essentially two ways.

  • dynamic - fetch this information over network
  • static - inline this information during build

I think @garbas will know what is the right call more than me. However I agree that hard-coding this is won't be scalable. But I'm ok resolving this in separate issue/PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would be done as a counter that counts from 0 to a number which is fetched from the backend meanwhile. But for now this was the easier hack.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a Count API that we can use from elasticsearch https://www.elastic.co/guide/en/elasticsearch/reference/current/search-count.html

@samueldr
Copy link
Member

samueldr commented Jan 12, 2021

Don't let this shower of papercuts discourage you. In reality this PR are good steps towards improvement.

@garbas
Copy link
Member Author

garbas commented Jan 15, 2021

@samueldr Thank you a ton! I really appreciate the deep review. I'll try to address reported issues today or tomorrow. Thank you again.

@turboMaCk turboMaCk self-requested a review January 15, 2021 08:01
@github-actions
Copy link
Contributor

Copy link
Member

@hugolgst hugolgst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are my few comments:

  • The search icon looks weird for me:
  • I find it confusing that the "Show more detail" text is on the separator:
  • Could be great to underline the items where you can click, otherwise, it won't be obvious:
    image
  • After clicking a title, the focus outline stays:
    image

Otherwise, it looks great!

@turboMaCk
Copy link
Member

@hugolgst which browser is it?

This is how it looks for me in FF

nixos-search

@hugolgst
Copy link
Member

I'm using google-chrome in i3-wm (don't know if this has a role)

@garbas
Copy link
Member Author

garbas commented Jan 23, 2021

Another smaller CSS issue which we should preferably solve

This is now fixed

@github-actions
Copy link
Contributor

@garbas
Copy link
Member Author

garbas commented Jan 23, 2021

@hugolgst

Here are my few comments:

* The search icon looks weird for me:
  ![](https://camo.githubusercontent.com/479c76aa4e57992e2fd5b2defbd19d17d3baa4f1bf2e51936630cdb890911e75/68747470733a2f2f692e696d6775722e636f6d2f6557456b4b75542e706e67)

I'm not sure how to do this better without importing additional font. I would leave this for now since I plan to work on redesign next. And there we will make sure this kind of things don't happen.

* I find it confusing that the "Show more detail" text is on the separator:
  ![](https://camo.githubusercontent.com/3ed093b1a385a0546fe4e64fc6d179663aed0503440148906861f30b6036b823/68747470733a2f2f692e696d6775722e636f6d2f3254466c6834432e706e67)

I also find it confusing. But I don't have better idea - at this point - how to solve it. As for the previous item I would table this until the redesign.

* Could be great to underline the items where you can click, otherwise, it won't be obvious:
  ![image](https://user-images.githubusercontent.com/15371828/105384436-80807100-5c12-11eb-8bda-1c6bbed4d48a.png)

This is somehow connected with the previous item since it is about displaying all the fields of a package. When redesigning (implementing the same design as nixos.org) I will look how to improve this.

* After clicking a title, the focus outline stays:
  ![image](https://user-images.githubusercontent.com/15371828/105384561-aa399800-5c12-11eb-8075-828077d7e4d6.png)

I'm also not able to reproduce this outline. I wonder if this is something specific to you. Can you debug it and see which style change would fix it?

@garbas
Copy link
Member Author

garbas commented Jan 23, 2021

@hugolgst

* After clicking a title, the focus outline stays:
  ![image](https://user-images.githubusercontent.com/15371828/105384561-aa399800-5c12-11eb-8075-828077d7e4d6.png)

I'm also not able to reproduce this outline. I wonder if this is something specific to you. Can you debug it and see which style change would fix it?

I just noticed you saif you were using chrome. I can see this being a problem there. let me fix it.

@hugolgst
Copy link
Member

hugolgst commented Jan 23, 2021

Thanks for the answers @garbas.
For the last point I added this CSS property to fix it.

outline: none;

@github-actions
Copy link
Contributor

@turboMaCk
Copy link
Member

@hugolgst

Thanks for the answers @garbas.
For the last point I added this CSS property to fix it.

outline: none;

This change is undesirable because outline is needed for keyboard navigation. See #258

@github-actions
Copy link
Contributor

@garbas
Copy link
Member Author

garbas commented Jan 23, 2021

Regarding the mobile stuff I will implement them also with the redesign. At the current state it would just take to long to implement this properly. The sooner I merge this the sooner I will start working on redesign.

@garbas garbas merged commit 5bb94c9 into main Jan 23, 2021
@garbas garbas deleted the faceted-search branch January 23, 2021 16:25
@turboMaCk
Copy link
Member

I'm also not able to reproduce this outline. I wonder if this is something specific to you. Can you debug it and see which style change would fix it?

This should be reproducible in any chromium based browser which all have default rule for this.

image

While I agree it doesn't look nice I think we would went into rabbit hole if we would try to fix all these things within the scope of the PR.

I would suggest:

  • The search icon looks weird for me

It took me a while to figure out what this is about because from screenshot it was not apparent where is this Q thing placed. Anyway I got it.
image

I think this is yet another regression caused by 4d505f3 (the font awesome removal). Adding font awesome will make content: "\2315"; work in chrome.

  • I find it confusing that the "Show more detail" text is on the separator

I personally don't find it confusing but if some does it should be addressed. But I would take it as a consideration for redesign rather than try to design it within this PR

  • After clicking a title, the focus outline stays

I don't think we should try to solve these small things in scope of this. We need outline for keyboard navigation. I would suggest opening separate issue to improve outline styling just so we don't forget to make sure this is improved in scope of redesign.

@turboMaCk
Copy link
Member

I was looking into icon/font-face. It turned out that in FireFox this is using FreeSherif font face. It's GNU licensed font.

adding font-family: FreeSherif to the before element fixes the problem provided this font is available on host system. Full fix though would require us to create webfont (which I already did) and load so we can display this correctly on all systems. However our current build system (webpack) doesn't support that. There are in fact some traces of configuration for static files but other parts of missing. yarn build doesn't include any assets into dist. This means that fixing this would require changes in build configuration which makes this a bit of a scope creep.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-6/11195/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

faceted search for packages
5 participants