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

Show datatype next to column names in advanced options when adding triggers #1188

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 2 commits into from
Dec 14, 2018
Merged

Show datatype next to column names in advanced options when adding triggers #1188

merged 2 commits into from
Dec 14, 2018

Conversation

@pthm
Copy link
Contributor

@pthm pthm commented Dec 11, 2018

Description

What component does this PR affect?

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Requires changes from other components? If yes, please mark the components:

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Related Issue

No related issue

Solution and Design

Small change to show the column datatype in the "Listen columns for update" section of the Advanced Options when creating new triggers.

After going through the process of creating a bulk load of triggers i found myself having to check columns data types with Postico to ensure I was enabling/disabling the right ones, Showing the datatype here would have been useful as it cuts out some of that workload

screenshot 2018-12-11 at 18 27 52

Would have liked to spend more time on how this is displayed as I'm not sure displaying it in-line next to the column name is the best way of showing this data but I am unfamiliar with the CSS stack used in the console so didn't want to play with the style system.

Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Docs update
  • Community content

Checklist:

  • I have read the contributing guide and my code conforms to the guidelines.
  • This change requires a change in the documentation.
  • I have updated the documentation accordingly.
  • I have added required tests.

@hasura-bot
Copy link
Contributor

hasura-bot commented Dec 11, 2018

Beep boop! 🤖

Hey @pthm, thanks for your PR!

One of my human friends will review this PR and get back to you as soon as possible. 🕐

Stay awesome! 😎

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@hasura-bot
Copy link
Contributor

hasura-bot commented Dec 11, 2018

Review app for commit 312ccad deployed to Heroku: https://hge-ci-pull-1188.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1188-312ccad

@dsandip
Copy link
Member

dsandip commented Dec 12, 2018

@pthm Thank you for the PR!

@rikinsk @shahidhk From the various screenshots of this page that I've seen, looks like people have more columns that we usually test with ;) and this is a pretty legit issue. However, I am personally not a fan of the way the column types are shown here and would prefer a table to show the name, type along with a checkbox.

@0x777 0x777 requested a review from praveenweb December 13, 2018 06:49
@0x777
Copy link
Member

0x777 commented Dec 13, 2018

@dsandip Let's just get this in? A different UI would require mockups, CSS changes etc. This looks neat and functional.

@dsandip
Copy link
Member

dsandip commented Dec 13, 2018 •
edited
Loading

@0x777 sounds good. Over to you @karthikvt26/ @praveenweb for reviewing (and @shahidhk if you have any comments).

shahidhk
shahidhk approved these changes Dec 14, 2018
View reviewed changes
Copy link
Member

@shahidhk shahidhk 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.

I agree with @dsandip, we need a polished UI here. But, as @0x777 said, let's get this merged and plan for a better UI here.

Thanks a lot @pthm 😄

@hasura-bot
Copy link
Contributor

hasura-bot commented Dec 14, 2018

Review app for commit 40ebf75 deployed to Heroku: https://hge-ci-pull-1188.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1188-40ebf75

@shahidhk shahidhk merged commit e3e57ff into hasura:master Dec 14, 2018
@hasura-bot
Copy link
Contributor

hasura-bot commented Dec 14, 2018

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

@hasura-bot
Copy link
Contributor

hasura-bot commented Dec 14, 2018

Beep boop! 🤖

Whoa! 🎉 🎉 💃

GIF

Awesome work @pthm! 💪 🏆 All of us at Hasura ❤️ what you did.

Thanks again 🤗

@shark-h
Copy link
Member

shark-h commented Dec 14, 2018

Hey @pthm !
We'd love to send you some swag, could you please ping me on Discord (Shark#9863) ?

hasura-bot pushed a commit that referenced this pull request Oct 3, 2024
@danieljharvey @hasura-bot
Move commands from sql to plan (#1188)
e313123 
<!-- The PR description should answer 2 important questions: -->

### What

Move the function/procedure planning from `sql` to the shared OpenDD IR
pipeline in `plan`. This should be a no-op for `sql`

### How

Move code, fix type errors.

V3_GIT_ORIGIN_REV_ID: 7da797ffedbc40a44692670679aa176817f2c65e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shahidhk shahidhk shahidhk approved these changes

@karthikvt26 karthikvt26 karthikvt26 approved these changes

@praveenweb praveenweb Awaiting requested review from praveenweb

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@pthm @hasura-bot @CLAassistant @dsandip @0x777 @shark-h @shahidhk @karthikvt26