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

Search: Query string syntax help #1828

Merged
merged 59 commits into from
Oct 2, 2020
Merged

Search: Query string syntax help #1828

merged 59 commits into from
Oct 2, 2020

Conversation

fiskus
Copy link
Member

@fiskus fiskus commented Sep 28, 2020

Description

Added

  • Help dropdown
  • Use of react-intl for text

May pay attention to

  • Hardcoded height for Search's wrapper
  • Created more small components instead of less large ones, they are simpler to maintain, mix and reuse

@fiskus fiskus added the javascript Pull requests that update Javascript code label Sep 28, 2020
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2020

Codecov Report

Merging #1828 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1828   +/-   ##
=======================================
  Coverage   89.49%   89.49%           
=======================================
  Files          62       62           
  Lines        7233     7233           
=======================================
  Hits         6473     6473           
  Misses        760      760           
Flag Coverage Δ
#api-python 87.87% <ø> (ø)
#lambda 92.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 b587d08...9b5f475. Read the comment docs.

@fiskus fiskus requested review from nl0 and akarve September 28, 2020 14:32
Comment on lines 61 to 68
'@keyframes appear': {
'0%': {
transform: 'translateY(-10px)',
},
'100%': {
transform: 'translateY(0)',
},
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Can replace it by <M.Slide /> or remove it completely, of course

@@ -0,0 +1,87 @@
{
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a lot of text, so I decided to move it outside of component

catalog/app/containers/NavBar/Search.js Show resolved Hide resolved
@fiskus
Copy link
Member Author

fiskus commented Sep 28, 2020

I didn't move the close button from SearchBox to dropdown yet.

The close button purpose now is to collapse SearchBox and reset the input query. We can't collapse SearchBox on blur in this implementation, because click on the dropdown triggers a blur event.
The dropdown can be closed by clicking outside of it or by focusing on SearchBox.

"searchQuerySyntax.objectAndPackage": " (objects & packages)",
"searchQuerySyntax.operators.and.title": "Conjunction",
"searchQuerySyntax.operators.exists.example": "_exists_:content",
"searchQuerySyntax.operators.exists.title": "Has any non-null value for keyword",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"searchQuerySyntax.operators.exists.title": "Has any non-null value for keyword",
"searchQuerySyntax.operators.exists.title": "Matches any non-null value for the given field",

"searchQuerySyntax.operators.and.title": "Conjunction",
"searchQuerySyntax.operators.exists.example": "_exists_:content",
"searchQuerySyntax.operators.exists.title": "Has any non-null value for keyword",
"searchQuerySyntax.operators.group.title": "Group items",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"searchQuerySyntax.operators.group.title": "Group items",
"searchQuerySyntax.operators.group.title": "Group terms",

Copy link
Member

@akarve akarve left a comment

Choose a reason for hiding this comment

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

Simplified help text

catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
@akarve akarve requested review from akarve and nl0 October 1, 2020 22:36
Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

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

  1. some more inline comments

  2. i've noticed a suboptimal ux when clicking on the search button when searchbox is collapsed -- the box gets expanded, but it feels kinda twitchy

  3. inconsistent margin / border (top and bottom) around the search button

Screenshot 2020-10-02 at 10 30 23

@@ -0,0 +1,313 @@
import cx from 'classnames'
import * as Lab from '@material-ui/lab'
Copy link
Member

Choose a reason for hiding this comment

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

lab should come after core (A-Z) and scoped packages should come after non-scoped

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. I think that sorting rule should be operated by eslint https://eslint.org/docs/rules/sort-imports

catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
setExpanded(false)
setHelpOpened(false)
if (onBlur) onBlur()
})
Copy link
Member

Choose a reason for hiding this comment

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

add onBlur to deps

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added 'react-hooks/exhaustive-deps': 2 to .eslintrc

catalog/app/containers/NavBar/Search.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Search.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
Copy link
Member Author

@fiskus fiskus left a comment

Choose a reason for hiding this comment

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

some more inline comments

Resloved them all but one about import sorting.

a suboptimal ux... twitchy

I removed slide animation then. If animation is not smooth, I think it shouldn't be there. This dropdown has a lot of things, and it's heavy. I can improve performance by caching its content later

inconsistent margin / border

I don't recognize it unfortunately. Can you elaborate? Can I fix it as bug in another PR?

@@ -0,0 +1,313 @@
import cx from 'classnames'
import * as Lab from '@material-ui/lab'
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. I think that sorting rule should be operated by eslint https://eslint.org/docs/rules/sort-imports

catalog/app/containers/NavBar/Help.js Outdated Show resolved Hide resolved
setExpanded(false)
setHelpOpened(false)
if (onBlur) onBlur()
})
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added 'react-hooks/exhaustive-deps': 2 to .eslintrc

@@ -3,20 +3,33 @@ import { push } from 'connected-react-router/esm/immutable'
import * as React from 'react'
import * as redux from 'react-redux'
import * as M from '@material-ui/core'
import * as Lab from '@material-ui/lab'
Copy link
Member Author

Choose a reason for hiding this comment

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

It is already. Did I miss something?

@fiskus
Copy link
Member Author

fiskus commented Oct 2, 2020

1. some more inline comments

done

2. i've noticed a suboptimal ux when clicking on the search button when searchbox is collapsed -- the box gets expanded, but it feels kinda twitchy

I removed animation for now. But will fix it with another PR: will delay dropdown animation for sure, and try something more to make it smoother

3. inconsistent margin / border (top and bottom) around the search button

Will fix border issues in another PR. I will play with margins too

@fiskus fiskus merged commit b173499 into master Oct 2, 2020
@fiskus fiskus deleted the search-fields-query-string branch October 2, 2020 17:36
@@ -72,6 +69,7 @@ module.exports = {
'react-hooks/rules-of-hooks': 'error',
'redux-saga/no-yield-in-race': 2,
'redux-saga/yield-effects': 2,
'react-hooks/exhaustive-deps': 2,
Copy link
Member

Choose a reason for hiding this comment

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

@fiskus this is nice, but i think we should keep the A-Z order for consistency

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. Will fix in the next PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants