这是indexloc提供的服务，不要输入任何密码
Skip to content

Add readable terms for the operators #3699

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 17, 2020
Merged

Add readable terms for the operators #3699

merged 5 commits into from
Jan 17, 2020

Conversation

@beerose
Copy link
Contributor

@beerose beerose commented Jan 14, 2020

Description

In Browse rows, there were listed postgres operators. This PR adds readable english terms and the operator in brackets so that anybody can understand it quickly.

Screenshot 2020-01-14 at 11 38 03

Affected components

  • Console

Breaking changes

  • No Breaking changes

@beerose beerose added the c/console Related to console label Jan 14, 2020
@beerose beerose requested a review from rikinsk as a code owner January 14, 2020 10:42
@beerose beerose self-assigned this Jan 14, 2020
@claassistantio
Copy link

claassistantio commented Jan 14, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Jan 14, 2020 •
edited
Loading

Deploy preview for hasura-docs ready!

Built with commit ed6d2df

https://deploy-preview-3699--hasura-docs.netlify.com

@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 14, 2020

Review app for commit 061ff70 deployed to Heroku: https://hge-ci-pull-3699.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3699-061ff70f

@coco98
Copy link
Contributor

coco98 commented Jan 14, 2020

@beerose @rikinsk Should we change ilike to like (ignore case) or like (case insensitive)?

@beerose beerose force-pushed the readable-operator-names branch from 061ff70 to 17ce506 Compare January 14, 2020 11:09
@beerose
Copy link
Contributor Author

beerose commented Jan 14, 2020

Yeah, it makes sense since we want to make it much more readable. I think I'd rather go with (case insensitive) since it's more common.

But take into account that it would quite widen the dropdown:

Screenshot 2020-01-14 at 12 19 43

@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 14, 2020

Review app for commit 17ce506 deployed to Heroku: https://hge-ci-pull-3699.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3699-17ce506e

rikinsk
rikinsk requested changes Jan 15, 2020
View reviewed changes
Copy link
Member

@rikinsk rikinsk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • Lets change the operator name to be of the format - [<graphql equivalent operator>] logical name. e.g. [_eq] equals. The $eq value (that is used by the v1/query request underneath) is not helpful to the user
  • Lets only display the operator name after doing the above
  • ilike -> like (case-insensitive) for clarity

It would also be great if we could somehow indicate that like and ilike expect values such as %search-term% to search anywhere. Maybe preset the value field with %% when they are selected?

@beerose
Copy link
Contributor Author

beerose commented Jan 15, 2020

I've changed dropdown options to be of the format [<graphql equivalent operator>] logical name and added (case insensitive) to ilike and nilike.

I also added the default value %% when like, nlike, ilike or nilike is chosen. Small demo:
likes

@beerose beerose requested a review from rikinsk January 15, 2020 21:10
@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 15, 2020

Review app for commit 0542286 deployed to Heroku: https://hge-ci-pull-3699.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3699-05422869

rikinsk
rikinsk requested changes Jan 16, 2020
View reviewed changes
Copy link
Member

@rikinsk rikinsk left a comment

Choose a reason for hiding this comment

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

I was just testing this out and realised we have another bug that we can resolve here.
For _in & _nin, we cast the value to string and the query breaks. The typecasting needs to be handled for them.

Also it might make sense add the default value as another field in the Operators constant itself so that we can keep adding other defaults easily. For example, for _in & _nin we can add default as empty array.

console/src/components/Services/Data/constants.js Outdated
{ name: 'not like', value: '$nlike', graphqlOp: '_nlike' },
{ name: 'like (case-insensitive)', value: '$ilike', graphqlOp: '_ilike' },
{
name: 'not ilike (case-insensitive)',
Copy link
Member

@rikinsk rikinsk Jan 16, 2020

Choose a reason for hiding this comment

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

not ilike -> not like

console/src/components/Services/Data/constants.js Outdated
graphqlOp: '_nilike',
},
{ name: 'similar', value: '$similar', graphqlOp: '_similar' },
{ name: 'not similar', value: '$nsimilar', graphqlOp: '_similar' },
Copy link
Member

@rikinsk rikinsk Jan 16, 2020

Choose a reason for hiding this comment

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

_similar -> _nsimilar

@beerose
Copy link
Contributor Author

beerose commented Jan 17, 2020

I handled the $in and $nin case. Also, in the future, we could think about adding validation -- validate if, for expected integer, there's a number, for expected array there's a valid array, etc.

@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 17, 2020

Review app for commit 0d485a0 deployed to Heroku: https://hge-ci-pull-3699.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3699-0d485a01

@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 17, 2020

Review app for commit 34e8782 deployed to Heroku: https://hge-ci-pull-3699.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3699-34e8782e

@beerose beerose requested a review from rikinsk January 17, 2020 08:43
@rikinsk rikinsk self-requested a review January 17, 2020 09:00
@rikinsk rikinsk merged commit edb17e8 into hasura:master Jan 17, 2020
@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 17, 2020

Review app https://hge-ci-pull-3699.herokuapp.com is deleted

@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 17, 2020

Review app for commit ed6d2df deployed to Heroku: https://hge-ci-pull-3699.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3699-ed6d2df0

polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020
@polRk
use user friendly terms for filter operators in browse rows page (has…
201b48f 
…ura#3699)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rikinsk rikinsk Awaiting requested review from rikinsk

Assignees

@beerose beerose

Labels

c/console Related to console

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@beerose @claassistantio @hasura-bot @coco98 @rikinsk